Fix broken log streaming from #35677 - #36127
Merged
Merged
Conversation
…ogging=True option in DockerSwarmOperator" It introduces logging of Docker Swarm services which was previously not working.
…standard and provided upstream by the Docker API. Therefore in DockerSwarmOperator follow is always false.
… API for logs. As we indicated in the previous commmit, the docker client malfunctions when we try to get the logs with follow=True. Therefore we make multiple calls to the API (every 2 seconds), to fetch the new logs.
…6 instead of 5) as we check if the service has terminated (+1). As this assertion makes less sense in a situation where we do multiple calls to the Docker API (polling), we might think of removing it or replacing it with something more suitable.
…in the Docker Swarm Operator. We log two times a different message and we assert that the two lines are given back in the logs in the expected sequence.
docker.errors.APIError: 503 Server Error for http+docker://localhost/v1.43/services/create: Service Unavailable ("This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again.")
Revert "Final commit of this PR marking the test case that validates logging in the Docker Swarm Operator. We log two times a different message and we assert that the two lines are given back in the logs in the expected sequence."
This reverts commit 048ba1e.
…ogging=True option in DockerSwarmOperator" It introduces logging of Docker Swarm services which was previously not working.
…standard and provided upstream by the Docker API. Therefore in DockerSwarmOperator follow is always false.
… API for logs. As we indicated in the previous commmit, the docker client malfunctions when we try to get the logs with follow=True. Therefore we make multiple calls to the API (every 2 seconds), to fetch the new logs.
…6 instead of 5) as we check if the service has terminated (+1). As this assertion makes less sense in a situation where we do multiple calls to the Docker API (polling), we might think of removing it or replacing it with something more suitable.
…in the Docker Swarm Operator. We log two times a different message and we assert that the two lines are given back in the logs in the expected sequence.
docker.errors.APIError: 503 Server Error for http+docker://localhost/v1.43/services/create: Service Unavailable ("This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again.")
Revert "Final commit of this PR marking the test case that validates logging in the Docker Swarm Operator. We log two times a different message and we assert that the two lines are given back in the logs in the expected sequence."
This reverts commit 048ba1e.
Member
|
What is the effect of it (i.e. is it catastrophic for users? We have two options - remove docker provider from RC1 and do RC2 with it or release it and make an ad-hoc release afterwards - depending on the answer :) And no worries, no need to apologize. it happens (and multuiple times) to all of us. And it's actually great you took it for a spin and tested, this is precisely what our 3 days voting process is aiming for as well - to catch and either hold those affected providers or to know we need to release a fix. |
Contributor
Author
|
Yeah so what happens is we stream each log line twice as "last_line_logged" was not being updated. On you to decide how to proceed to avoid also additional efforts by removing :) Cheers |
Member
|
I think it's important-enough to RC2 - but it's @eladkal 's decision (Elad is release manager) |
potiuk
approved these changes
Dec 8, 2023
potiuk
changed the title
hussein-awala
approved these changes
Dec 8, 2023
eladkal
approved these changes
Dec 9, 2023
Contributor
|
I will cancel RC1 for docker peovider and cut rc2 |
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes broken log streaming introduced in #35677