Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CI compose version 1.29.2 / 2.2.3 #1251

Merged
merged 3 commits into from
Jan 24, 2022

Conversation

aminvakil
Copy link
Collaborator

@aminvakil aminvakil commented Jan 11, 2022

If it's causing issues (#1133 (comment)) why should we stick to it?

Also while we're here let's bump docker-compose 1.x version to latest release.

So this PR got changed in the way and at last min/max 1.x and 2.x versions of docker-compose added to CI as suggested by @chadwhitacre in #1251 (comment)

Also because of recent changes in 2.2.3 (docker/compose#9035) -T had to be added to some scripts using $dcr where output of it was getting used.

  -T, --no-TTY                Disable pseudo-noTty allocation. By default docker compose run allocates a TTY

@aminvakil aminvakil changed the title Bump CI compose version 2.2.3 Bump CI compose version 1.29.2 / 2.2.3 Jan 11, 2022
@chadwhitacre
Copy link
Member

Revert? 👀

@aminvakil
Copy link
Collaborator Author

Revert? eyes

Trying to see if the problem happens again with v2.0.1 or what? Sorry for the noise.

@chadwhitacre
Copy link
Member

Ah, gotcha. I figured it was #1171. :-/

@aminvakil
Copy link
Collaborator Author

I'm running self-hosted (21.11.0) with docker compose v2.2.3 and I don't have a problem...

@aminvakil
Copy link
Collaborator Author

aminvakil commented Jan 18, 2022

OK, So I went and checked this PR a couple of times during the past few days and couldn't figure out what is wrong with 2.2.3, last solution I have is to check every release between 2.0.1 and 2.2.3 to see what breaks where :) and then figure out why

CI Passed:

  • 2.0.1
  • 2.1.0 (not tested)
  • 2.1.1
  • 2.2.1
  • 2.2.2
  • 2.2.3

So the regression starts with 2.2.3 (https://github.com/docker/compose/releases/tag/v2.2.3) itself.

@aminvakil
Copy link
Collaborator Author

sentry-self-hosted-relay-1                                      "/bin/bash /docker-e…"   relay                                      restarting          

relay stucks in a loop:

2022-01-18T13:22:01.7151325Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7152059Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0
2022-01-18T13:22:01.7152692Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7153321Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0
2022-01-18T13:22:01.7153963Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7154568Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0
2022-01-18T13:22:01.7155214Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7155841Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0
2022-01-18T13:22:01.7156460Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7157079Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0
2022-01-18T13:22:01.7157698Z sentry-self-hosted-relay-1                                     | error: could not parse json config file (file /work/.relay/credentials.json)
2022-01-18T13:22:01.7158316Z sentry-self-hosted-relay-1                                     |   caused by: EOF while parsing a value at line 1 column 0

@BYK
Copy link
Member

BYK commented Jan 18, 2022

@aminvakil could this be some sort of Unix/Windows line ending issue?

@BYK
Copy link
Member

BYK commented Jan 18, 2022

@aminvakil, actually, I bet docker compose 2.2.3 is outputting some random/garbage values in the run output at this step:

$dcr \
--no-deps \
--volume "$(pwd)/$RELAY_CONFIG_YML:/tmp/config.yml" \
relay --config /tmp credentials generate --stdout \
> "$RELAY_CREDENTIALS_JSON"

Producing an invalid YAML, causing relay to fail. That means getsentry/relay#1169 is invalid and we should address the issue here or with docker compose itself directly.

@untitaker
Copy link
Member

untitaker commented Jan 18, 2022

@BYK are there any issues with not using --stdout and generating the credentials in a mounted volume?

I think the bug described in the comment above might also purely exist because you're redirecting stdout to a file instead of letting Relay create the file, because that creates an empty file before the relay process starts.

@BYK
Copy link
Member

BYK commented Jan 18, 2022

@untitaker using a volume is fine here but Relay does not like that scenario: getsentry/relay#564

@aminvakil
Copy link
Collaborator Author

aminvakil commented Jan 19, 2022

I removed putting output to file to see what is the output in 6366592.
It seems that nothing gets generated at all using 2.2.3
https://github.com/getsentry/self-hosted/runs/4865126485?check_suite_focus=true#step:4:498
Same job with 1.29.2 though:
https://github.com/getsentry/self-hosted/runs/4865126429?check_suite_focus=true#step:4:717

I guess it's related to docker/compose#9035 which fixed another bug reported by @chadwhitacre :)

@untitaker
Copy link
Member

using a volume is fine here but Relay does not like that scenario

That linked PR only changes relay's behavior when using the --stdout flag, but to be clear i propose not using that flag, not using > and letting relay create the credentials file itself, not the shell

@aminvakil
Copy link
Collaborator Author

Closing and reopening to see if kafka thing is temporary or not.

@aminvakil aminvakil closed this Jan 19, 2022
@aminvakil aminvakil reopened this Jan 19, 2022
@BYK
Copy link
Member

BYK commented Jan 19, 2022

@aminvakil let's try what @untitaker suggest and remove the --stdout flag?

@chadwhitacre
Copy link
Member

Oh wow I've been missing the party. 🍿

@aminvakil
Copy link
Collaborator Author

aminvakil commented Jan 23, 2022

@BYK @chadwhitacre Sorry for answering late. I wanted to give myself more time to think about this.

@aminvakil let's try what @untitaker suggest and remove the --stdout flag?

I propose keeping this PR as it is, but create another PR which changes relay credentials generation as @untitaker suggests, and take a look at this PR after that has been changed, what do you think?

what do you think of testing against more than two versions of Docker Compose? How about min/max of v1 and min/max of v2? So ...

Sure, why not? I say add them later after merging the relay credential PR.

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If -T fixes things, I actually like this approach more. Should we bake that into the $dcr shortcut?

Also, we either need to add the old versions to the matrix too or update the required checks in branch protection rules @chadwhitacre

@aminvakil
Copy link
Collaborator Author

If -T fixes things, I actually like this approach more. Should we bake that into the $dcr shortcut?

I'm not sure how it would act on older docker-compose versions or different parts which $dcr is getting used, I say let's not change everything everywhere :) (at least for now)

@BYK
Copy link
Member

BYK commented Jan 24, 2022

I say let's not change everything everywhere :) (at least for now)

Then please explain why that is needed on the kafka thing or revert that? 🤣

@aminvakil aminvakil closed this Jan 24, 2022
@aminvakil aminvakil reopened this Jan 24, 2022
@aminvakil aminvakil force-pushed the ci_docker_compose_v2.2.3 branch 2 times, most recently from b36708d to f34f46f Compare January 24, 2022 11:45
@aminvakil
Copy link
Collaborator Author

Last failed run was because of docker/compose#6704

@chadwhitacre
Copy link
Member

Interesting, docker/compose#9035, which introduced the need for -T w/ 2.2.3, was the fix for #1133. Oh the tangled web we weave. 😂 😭

Copy link
Member

@chadwhitacre chadwhitacre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dig it! 👍

@chadwhitacre chadwhitacre merged commit 3abbb3e into getsentry:master Jan 24, 2022
@chadwhitacre
Copy link
Member

👏 👏 @aminvakil as well as @BYK for wrestling this to the ground. It was supposed to be easy! 🙈

@aminvakil aminvakil deleted the ci_docker_compose_v2.2.3 branch January 24, 2022 17:49
@aminvakil
Copy link
Collaborator Author

@BYK Thank you for helping me through this!
@chadwhitacre Thanks for merging!

chadwhitacre added a commit that referenced this pull request Jan 24, 2022
chadwhitacre added a commit that referenced this pull request Jan 24, 2022
This was referenced Feb 1, 2022
chadwhitacre added a commit that referenced this pull request Feb 2, 2022
chadwhitacre added a commit that referenced this pull request Feb 2, 2022
chadwhitacre added a commit that referenced this pull request Feb 3, 2022
* Revert "Revert "Add CI compose version 1.29.2 / 2.0.1 / 2.2.3 (#1251)" (#1272)"

This reverts commit da8f490.

* Drop back to -T because long opt is Compose 2+

Admin-merging with approvals from outside contributors (All-seeing eye, are you seeing this? ;)
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants