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

switch to route-controller-manager image and use ApplyDeployment #258

Merged
merged 5 commits into from
Sep 29, 2022
Merged

switch to route-controller-manager image and use ApplyDeployment #258

merged 5 commits into from
Sep 29, 2022

Conversation

atiratree
Copy link
Member

@atiratree atiratree commented Aug 29, 2022 •
edited
Loading

following up on #255

  • use route-controller-manager image
  • remove deprecated ApplyDeploymentWithForce and ApplyDaemonSetWithForce

TODO:

@openshift-ci openshift-ci bot requested review from deads2k and soltysh August 29, 2022 22:08
@atiratree atiratree force-pushed the route-controller-manager-image branch from e71ccd9 to e716f9d Compare August 30, 2022 12:01
deads2k
deads2k reviewed Sep 2, 2022
View reviewed changes
pkg/operator/sync_openshiftcontrollermanager_v311_00.go
@@ -429,16 +447,11 @@ func manageOpenShiftControllerManagerDeployment_v311_00_to_latest(client appscli
}
newEnvs = append(newEnvs, env)
}
// reflect.DeepEqual does not consider this case equal
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

because the new approach is to do a hash of the whole spec: https://github.com/openshift/library-go/blob/245d1ca16d1514313f3a7ab13ff2c9e3b07b5f43/pkg/operator/resource/resourceapply/apps.go#L95

and compare that here https://github.com/openshift/library-go/blob/master/pkg/operator/resource/resourceapply/apps.go#L131

Copy link
Member

Choose a reason for hiding this comment

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

@knelasevero for awareness about this change. We need to do the same for https://github.com/openshift/cluster-kube-descheduler-operator/blob/master/pkg/operator/target_config_reconciler.go#L591 and https://github.com/openshift/secondary-scheduler-operator/blob/master/pkg/operator/target_config_reconciler.go#L276-L277. Would you please take a look?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will do

@deads2k
Copy link
Contributor

deads2k commented Sep 2, 2022

the change looks reasonable to me, modulo the question. I'll leave lgtm with @ingvagabund or delegate

/approve

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2022
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2022
@atiratree atiratree force-pushed the route-controller-manager-image branch from e716f9d to 869d5f8 Compare September 2, 2022 21:31
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2022
@atiratree
Copy link
Member Author

/retest

ingvagabund
ingvagabund approved these changes Sep 5, 2022
View reviewed changes
manifests/09_deployment.yaml Show resolved Hide resolved
pkg/operator/sync_openshiftcontrollermanager_v311_00.go
@@ -429,16 +447,11 @@ func manageOpenShiftControllerManagerDeployment_v311_00_to_latest(client appscli
}
newEnvs = append(newEnvs, env)
}
// reflect.DeepEqual does not consider this case equal
Copy link
Member

Choose a reason for hiding this comment

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

@knelasevero for awareness about this change. We need to do the same for https://github.com/openshift/cluster-kube-descheduler-operator/blob/master/pkg/operator/target_config_reconciler.go#L591 and https://github.com/openshift/secondary-scheduler-operator/blob/master/pkg/operator/target_config_reconciler.go#L276-L277. Would you please take a look?

@ingvagabund
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2022
@atiratree atiratree force-pushed the route-controller-manager-image branch from 869d5f8 to 5b4ef1a Compare September 5, 2022 14:32
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2022
@ingvagabund
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2022
@atiratree
switch to route-controller-manager image and update deployment spec
d6a388d
@atiratree
remove deprecated ApplyDeploymentWithForce in route-controller-manager
ccaeed5 
with ApplyDeployment
- newly reacts to all changes in Deployment spec  (eg cmd changes will
  be reacted upon)
- reacts to external changes like configmaps via spec annotations
@atiratree
remove deprecated ApplyDaemonSetWithForce in openshift-controller-man…
4bb4b52 
…ager

with ApplyDaemonSet
- newly reacts to all changes in DeamonSet spec  (eg cmd changes will
  be reacted upon)
- reacts to external changes like configmaps via spec annotation
@atiratree
fix tests
028c0fa
@atiratree atiratree force-pushed the route-controller-manager-image branch from 5b4ef1a to 028c0fa Compare September 8, 2022 17:22
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2022
@atiratree
Copy link
Member Author

/hold until there is a first OSBS build

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022
@atiratree
Copy link
Member Author

ci was failing, it seems SyncConfigMap can return nil in some cases - 5e87d41 should fix it

@atiratree
Copy link
Member Author

/retest

@atiratree
add nil checks for SyncConfigMap calls
2c88211 
- add err handling for ensureAtMostOnePodPerNodeFn
@atiratree atiratree mentioned this pull request Sep 21, 2022
Merged
1 task
@atiratree
Copy link
Member Author

we have the first build
/hold cancel
/retest

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2022
@ingvagabund
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, deads2k, ingvagabund

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

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [deads2k,ingvagabund]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@atiratree
Copy link
Member Author

/label docs-approved
/label qe-approved
/label px-approved

failures do not seem to be connected to this change
/retest-required

@openshift-ci openshift-ci bot added docs-approved Signifies that Docs has signed off on this PR qe-approved Signifies that QE has signed off on this PR px-approved Signifies that Product Support has signed off on this PR labels Sep 29, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 29, 2022

@atiratree: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/openshift-e2e-aws-builds-techpreview 2c88211 link false /test openshift-e2e-aws-builds-techpreview

Full PR test history. Your PR dashboard.

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 openshift-merge-robot merged commit 7fdbe73 into openshift:master Sep 29, 2022
@stbenjam stbenjam mentioned this pull request Sep 30, 2022
Merged
@atiratree atiratree mentioned this pull request Oct 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knelasevero knelasevero knelasevero left review comments

@deads2k deads2k deads2k left review comments

@ingvagabund ingvagabund ingvagabund approved these changes

@soltysh soltysh Awaiting requested review from soltysh

Assignees

@ingvagabund ingvagabund

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@atiratree @deads2k @ingvagabund @knelasevero @openshift-merge-robot