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.

Sign up for GitHub

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

Jump to bottom

apps: Fix dc triggers reconciliation on image change and do not deploy DCs with empty image #17539

Merged
merged 3 commits into from
Jan 12, 2018
Merged

apps: Fix dc triggers reconciliation on image change and do not deploy DCs with empty image #17539

merged 3 commits into from
Jan 12, 2018

Conversation

tnozicka
Copy link
Contributor

@tnozicka tnozicka commented Nov 30, 2017 •
edited
Loading

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2017
@gabemontero gabemontero removed their request for review November 30, 2017 19:13
mfojtik
mfojtik reviewed Dec 1, 2017
View reviewed changes
pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go Outdated

// Never deploy with invalid or unresolved images
for i, container := range config.Spec.Template.Spec.Containers {
if strings.TrimSpace(container.Image) == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

i thought we guard against this in shouldTrigger()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't, we wait for the RC to be created and because the image resolution/injection is happening asynchronously it will manage to deploy RC with "" before the image gets injected

Copy link
Contributor Author

@tnozicka tnozicka Dec 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I tried fixing the image in that function but then it will end up being triggered as config change. we could probably add exception for that as well but this felt cleanest and most reliable. We should do the same upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be better to move this up to triggerActivated and just return false, true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it could be there now that shouldSkip is honored

Copy link
Contributor Author

@tnozicka tnozicka Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@mfojtik actually that wouldn't work for manual rollouts without any trigger, so needs to stay here

@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 5, 2017

/retest

@openshift-ci-robot openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 5, 2017
@tnozicka tnozicka added kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Dec 5, 2017
@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 5, 2017

Added a fix for DC image reactor. Tests are green and ready for review.
/assign @mfojtik

@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 5, 2017

/cherrypick release-3.8
/cherrypick release-3.7

@openshift-cherrypick-robot
Copy link

@tnozicka: @tnozicka once the present PR merges, I will cherry-pick it on top of release-3.8
in a new PR and assign it to you.

In response to this:

/cherrypick release-3.8
/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tnozicka tnozicka changed the title apps: Fix dc triggers when patching empty image apps: Fix dc triggers reconciliation on image change and do not deploy DCs with empty image Dec 5, 2017
@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 5, 2017

/cherrypick release-3.7

@openshift-cherrypick-robot
Copy link

@tnozicka: @tnozicka once the present PR merges, I will cherry-pick it on top of release-3.7 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mfojtik
mfojtik reviewed Dec 8, 2017
View reviewed changes
pkg/image/trigger/deploymentconfigs/deploymentconfigs.go
// We need to react on image changes as well. Image names could change,
// images could be set to different value or resetted to "" e.g. by oc apply
// and we need to make sure those changes get reconciled by re-resolving images
case !reflect.DeepEqual(dc.Spec.Template.Spec.Containers, oldDC.Spec.Template.Spec.Containers):
Copy link
Contributor

Choose a reason for hiding this comment

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

should be check only the images then?

Copy link
Contributor

Choose a reason for hiding this comment

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

worst case this will be processed but not triggered, but I think we need some common helper like HasUpdatedImages() to work well for this and for other places. this is fine now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discussed on IRC but for the record: I don't see other way we can determine if it needs to be queued without actually doing all the work in UpdateDeploymentConfigImages. This is just a check for adding to work queue, it should be fast and simple. After picking it up from queue we already do all the checks for updated images and others.

Copy link
Contributor

Choose a reason for hiding this comment

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

What happens when I want to run my own image for sometime and not the image resolved by the trigger? Can I do that now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis set automatic to false
if it's active the state should always be reconciled

@mfojtik
Copy link
Contributor

mfojtik commented Dec 8, 2017

one last question, looks good to me otherwise

@mfojtik
Copy link
Contributor

mfojtik commented Dec 8, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 8, 2017
@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 8, 2017

/retest

1 similar comment
@tnozicka
Copy link
Contributor Author

tnozicka commented Dec 8, 2017

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@tnozicka
Copy link
Contributor Author

github down on clone
/retest

@tnozicka
Copy link
Contributor Author

/retest

1 similar comment
@tnozicka
Copy link
Contributor Author

/retest

@tnozicka tnozicka force-pushed the fix-dc-triggers-when-patching-empty-image branch from f782a9b to 0e45ee9 Compare January 10, 2018 14:26
@tnozicka
Copy link
Contributor Author

no changes, just squashed the commit with additional log message for tests

/hold cancel
we should be good to go and ready with logs if we see that flake again

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2018
@tnozicka
Copy link
Contributor Author

/retest

@tnozicka
Copy link
Contributor Author

infra issues
/retest

@tnozicka tnozicka force-pushed the fix-dc-triggers-when-patching-empty-image branch from 0e45ee9 to 94e0120 Compare January 11, 2018 12:38
@tnozicka
Copy link
Contributor Author

@mfojtik this is what was slowing down the ci/openshift-jenkins/cmd tests and timing out, should be fixed now

mfojtik
mfojtik reviewed Jan 11, 2018
View reviewed changes
pkg/apps/controller/deploymentconfig/deploymentconfig_controller.go Outdated
return c.updateStatus(config, existingDeployments)
// Never deploy with invalid or unresolved images
for i, container := range config.Spec.Template.Spec.Containers {
if strings.TrimSpace(container.Image) == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

len() :-)

Copy link
Contributor Author

@tnozicka tnozicka Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

fixed, hopefully for good now ;)

@tnozicka tnozicka force-pushed the fix-dc-triggers-when-patching-empty-image branch from 94e0120 to cda584a Compare January 11, 2018 13:53
@tnozicka
Copy link
Contributor Author

CI cluster network issues
/retest

@tnozicka
Copy link
Contributor Author

flake: #17882
/retest

@tnozicka
Copy link
Contributor Author

@mfojtik len() is there, tests are green - lgty?

@mfojtik
Copy link
Contributor

mfojtik commented Jan 12, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 12, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, tnozicka

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

/test all [submit-queue is verifying that this PR is safe to merge]

@openshift-ci-robot
Copy link

@tnozicka: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_conformance_install cda584a link /test extended_conformance_install

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 17539, 17457).

@openshift-merge-robot openshift-merge-robot merged commit 781b665 into openshift:master Jan 12, 2018
@openshift-cherrypick-robot
Copy link

@tnozicka: cannot checkout release-3.8
: error checking out release-3.8
: exit status 1. output: error: pathspec 'release-3.8?' did not match any file(s) known to git.

In response to this:

/cherrypick release-3.8
/cherrypick release-3.7

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tnozicka
Copy link
Contributor Author

/cherrypick release-3.8

@openshift-cherrypick-robot
Copy link

@tnozicka: #17539 failed to apply on top of branch "release-3.8":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	test/extended/deployments/deployments.go
M	test/extended/testdata/bindata.go
Falling back to patching base and 3-way merge...
Auto-merging test/extended/testdata/bindata.go
Auto-merging test/extended/deployments/deployments.go
CONFLICT (content): Merge conflict in test/extended/deployments/deployments.go
Patch failed at 0001 Add test to verify we don't deploy unspecified images in DC

In response to this:

/cherrypick release-3.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tnozicka tnozicka deleted the fix-dc-triggers-when-patching-empty-image branch January 14, 2018 20:04
@tnozicka tnozicka mentioned this pull request Jan 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@0xmichalis 0xmichalis 0xmichalis left review comments

@mfojtik mfojtik mfojtik left review comments

@deads2k deads2k Awaiting requested review from deads2k

Assignees

@mfojtik mfojtik

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. priority/P1 size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tnozicka @openshift-cherrypick-robot @mfojtik @openshift-bot @openshift-ci-robot @openshift-merge-robot @0xmichalis