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

fix (ecs): Unstable deployments for task definitions with more than one container (#5544) #4409

Merged
merged 2 commits into from
Mar 12, 2020

Conversation

atyutyunnik
Copy link
Contributor

@atyutyunnik atyutyunnik commented Mar 11, 2020

The original code will not finish deployment and will eventually time out, in spite of task being successfully deployed by ECS and running (spinnaker/spinnaker#5544)

    "containers": [:

        "containers": [
            {
                "networkBindings": [],
                "networkInterfaces": [],
            },
            {
                "lastStatus": "RUNNING",
                "networkBindings": [
                    {
                        "bindIP": "0.0.0.0",
                        "containerPort": 8080,
                        "hostPort": 32773,
                        "protocol": "tcp"
                    }
            }]

Copy link
Contributor

@allisaurus allisaurus left a comment

Choose a reason for hiding this comment

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

can we add unit tests to verify that tasks with network bindings in containers other than the first one are correctly evaluated?

@atyutyunnik
Copy link
Contributor Author

can we add unit tests to verify that tasks with network bindings in containers other than the first one are correctly evaluated?

Normally, that's the first thing I do when introducing a change, but I was quite in a hurry with the fix for my own purposes and I also need to figure out how these particular unit tests work.

@allisaurus
Copy link
Contributor

quoting @atyutyunnik from the slack convo about this issue:

I have re-verified it with the original 1.19.0 - the problem is there, UT detects it; applied the fix - UT is happy

🎉

@clareliguori clareliguori added the ready to merge Approved and ready for a merge label Mar 12, 2020
@mergify mergify bot added the auto merged Merged automatically by a bot label Mar 12, 2020
@mergify mergify bot merged commit 2a7ad04 into spinnaker:master Mar 12, 2020
@clareliguori
Copy link
Member

@spinnakerbot cherry-pick 1.19

spinnakerbot pushed a commit that referenced this pull request Mar 12, 2020
…r in task definition (#5544) (#4409)

Co-authored-by: Alexander Tyutyunnik <alexandert@edifecs.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@spinnakerbot
Copy link
Contributor

Cherry pick successful: #4416

ezimanyi pushed a commit that referenced this pull request Mar 12, 2020
…r in task definition (#5544) (#4409) (#4416)

Co-authored-by: Alexander Tyutyunnik <alexandert@edifecs.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

Co-authored-by: atyutyunnik <45210095+atyutyunnik@users.noreply.github.com>
Co-authored-by: Alexander Tyutyunnik <alexandert@edifecs.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@allisaurus
Copy link
Contributor

@spinnakerbot cherry-pick 1.18

@spinnakerbot
Copy link
Contributor

Cherry pick failed: Command failed (cherry pick commit 2a7ad04) with exit code 1:

error: could not apply 2a7ad04fe... fix (ecs): Unstable deployments when there are more than one container in task definition (#5544) (#4409)
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

@atyutyunnik
Copy link
Contributor Author

For 1.18, you have to use PR# 4411. PR4409 is not compatible with the code base

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for a merge target-release/1.20
Projects
None yet
4 participants