hack/cluster-push-prep.sh: override only needed objects #399

runcom · 2019-02-11T13:26:41Z

Close #259

First and third commits are auxiliary, the second one is the one actually adding overrides for the CVO in the hack/cluster-push-* scripts.

Could you test it out? It works fine on AWS.

Signed-off-by: Antonio Murdaca <runcom@linux.com>

openshift-ci-robot · 2019-02-11T13:26:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: runcom

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [runcom]

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

cgwalters · 2019-02-11T13:34:49Z

HACKING.md

But...we want to be able to develop on the operator too right?

setting unmanaged for the operator itself is already enough to change the image for the operator and and the deployment itself w/o the CVO stomping on us. If you read some lines above, this is just for the server,controller and daemon (unless I didn't make that clear)

(I'm retesting this flow on a new cluster though)

alright, worked as I described, if you overrides the Deployment for the operator, you can edit it w/o doing anything else (nor scaling) as the CVO ignores any change to it. e.g you can make deploy-operator just fine or oc edit deployment/machine-config-operator and the CVO doesn't stomp on you

I'm in favor of using overrides to be clear. My objection is to recommending scaling the operator.

yup, it makes sense indeed, I just grabbed that from the section below as it's really the way to actually develop at least the daemon w/o the operator messing with it

cgwalters · 2019-02-11T18:04:47Z

HACKING.md

But that's why the current make deploy- model bounces (scales down+up) the operator - because we don't wait to wait for it to notice the images change. (Though the slowness there must really be a bug right?)

I think this text is incorrect - we shouldn't recommend disabling the operator when developing things as some functionality depends on it...my recent osImageURL work depended on patching both the operator and daemon for example.

I'm fine with that indeed, we need to remove that wording from the section below as well though and since i.e. the daemon isn't CVO managed there's no real way to edit it without the operator replacing it with its own pristine copy

the daemon isn't CVO managed

The daemon is managed by the operator.

there's no real way to edit it without the operator replacing it with its own pristine copy

That's not true; the current make deploy- scripts have been doing this just fine right?

Again I think the problem is some sort of race condition where even changing images.json the operator doesn't notice the change right away and tries to change the deployment back based on its old cached copy.

there's no real way to edit it without the operator replacing it with its own pristine copy

That's not true; the current make deploy- scripts have been doing this just fine right?

I need to dig deeper but I think, from my testing, the current make deploy-* works just because you also patch the images and the operator re-syncing still result in the new built image but if you try editing something else (with the overrides) the daemon will be replaced again

Sorry to jump in, but I'm confused by what you wrote @runcom I dont currently use the hack/* tools but when I work on MC*, I only scale down the CVO, I've never had to do anything to the MCO. Once I scale, my changes to each image "stick" and I have no problems.

yeah, I was talking about moving away from scaling the CVO to zero and just set some objects to unmanaged!

runcom · 2019-02-11T19:20:44Z

Ok, I've dropped the last commit while I keep testing - rest should be ok to review and test I guess

runcom · 2019-02-11T19:21:29Z

I want to be sure that overrides vs disabling the CVO doesn't result in any development regression for any of you, so please, take this PR :P

runcom · 2019-02-11T20:26:34Z

flake

/test e2e-aws

runcom · 2019-02-11T22:05:49Z

hack/cluster-push-prep.sh

+# XXX: --type merge completely overrides any previous "overrides" array
+#      find a way to just append? json op: add isn't working at all
+#      if there's not an overrides array already, that's why we use merge
+oc patch clusterversions.config.openshift.io/version --type merge -p '{"spec":{"overrides": [{"kind": "Deployment","name": "machine-config-operator", "namespace": "openshift-machine-config-operator", "unmanaged": true}, {"kind": "ConfigMap","name": "machine-config-operator-images", "namespace": "openshift-machine-config-operator", "unmanaged": true}]}}'


I believe we don't need to add the Deployment for the operator here or we're going to miss the wiring with the CVO (status report and whatnot). @cgwalters is this your understanding as well?

runcom · 2019-02-11T22:13:46Z

Still poking with this

/hold

kikisdeliveryservice · 2019-11-27T20:03:27Z

/skip

openshift-ci-robot · 2019-11-27T21:56:12Z

@runcom: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`6e50ddb`	link	`/test e2e-aws`
ci/prow/e2e-aws-disruptive	`6e50ddb`	link	`/test e2e-aws-disruptive`
ci/prow/e2e-gcp-op	`6e50ddb`	link	`/test e2e-gcp-op`
ci/prow/e2e-aws-upgrade	`6e50ddb`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-gcp-upgrade	`6e50ddb`	link	`/test e2e-gcp-upgrade`
ci/prow/e2e-vsphere	`6e50ddb`	link	`/test e2e-vsphere`

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.

Details

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.

kikisdeliveryservice · 2019-12-03T22:33:01Z

In an effort to clean up the MCO repo, closing old open PRs with no recent activity.

Feel free to reopen.

runcom added 2 commits February 11, 2019 12:00

hack/cluster-push-*: silence curl

ed66865

Signed-off-by: Antonio Murdaca <runcom@linux.com>

hack/cluster-push-prep.sh: override only needed objects

6e50ddb

Signed-off-by: Antonio Murdaca <runcom@linux.com>

openshift-ci-robot requested review from crawford and kikisdeliveryservice February 11, 2019 13:26

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 11, 2019

cgwalters reviewed Feb 11, 2019

View reviewed changes

runcom force-pushed the cvo-overrides branch from 8b03572 to 7602517 Compare February 11, 2019 13:41

cgwalters reviewed Feb 11, 2019

View reviewed changes

runcom force-pushed the cvo-overrides branch from 7602517 to 6e50ddb Compare February 11, 2019 19:20

runcom commented Feb 11, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 11, 2019

cgwalters mentioned this pull request Feb 26, 2019

WIP: Add hack/cluster-cvo-push.sh #496

Closed

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2019

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2019

kikisdeliveryservice closed this Dec 3, 2019

kikisdeliveryservice added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 3, 2019

hack/cluster-push-prep.sh: override only needed objects #399

hack/cluster-push-prep.sh: override only needed objects #399

Uh oh!

Conversation

runcom commented Feb 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Feb 11, 2019

Uh oh!

runcom commented Feb 11, 2019

Uh oh!

runcom commented Feb 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Feb 11, 2019

Uh oh!

kikisdeliveryservice commented Nov 27, 2019

Uh oh!

openshift-ci-robot commented Nov 27, 2019

Uh oh!

kikisdeliveryservice commented Dec 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

runcom commented Feb 11, 2019 •

edited

Loading