Skip to content

add empty CRs #15

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
Closed
deads2k wants to merge 1 commit into from
Closed

add empty CRs #15

deads2k wants to merge 1 commit into from

Conversation

@deads2k
Copy link
Contributor

@deads2k deads2k commented Feb 12, 2019

@bparees since you asked

I think this will avoid stomping. The test-aws should fail if this clears values.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 12, 2019
@bparees
Copy link
Contributor

bparees commented Feb 12, 2019

/lgtm
🤞

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019
@bparees
Copy link
Contributor

bparees commented Feb 12, 2019

if this works we'll want to follow it up w/ one to create the default build resource

@deads2k
Copy link
Contributor Author

deads2k commented Feb 12, 2019

/retest

@deads2k deads2k added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 13, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Feb 13, 2019

/hold

thinking about the impact of this when combined with the render step.

@openshift-ci-robot 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 13, 2019
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 14, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

aws

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

/hold cancel

@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 Feb 14, 2019
@bparees
Copy link
Contributor

bparees commented Feb 14, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
@deads2k deads2k added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

the dreaded sha sha

@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

/test e2e-aws

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Feb 14, 2019

@deads2k including these in /manifests means that CVO will empty out all the CRs continuously....

@bparees
Copy link
Contributor

bparees commented Feb 14, 2019

@deads2k including these in /manifests means that CVO will empty out all the CRs continuously....

it's not merging/applying?

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Feb 14, 2019

@deads2k including these in /manifests means that CVO will empty out all the CRs continuously....

it's not merging/applying?

the /manifests is the desired, which according to this PR you want empty...

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Feb 14, 2019

@deads2k including these in /manifests means that CVO will empty out all the CRs continuously....

it's not merging/applying?

the /manifests is the desired, which according to this PR you want empty...

/hold

@openshift-ci-robot 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 14, 2019
@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Feb 14, 2019

@deads2k including these in /manifests means that CVO will empty out all the CRs continuously....

it's not merging/applying?

https://github.com/openshift/cluster-version-operator/blob/0619e6cd016a14a0952542838b7f95eff2ddae43/pkg/cvo/internal/generic.go#L29-L58

@deads2k
Copy link
Contributor Author

deads2k commented Feb 14, 2019

We'll sort it out tomorrow. For items in config.openshift.io, I'm sure we can work out a spec/status update split.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 9, 2019

Hm, I am confused how this intersects with the MCO. I see https://github.com/openshift/installer/blob/83ca64d34e7e16935cbaa39fb42a6c8ba9d58c33/data/data/bootstrap/files/usr/local/bin/bootkube.sh.template#L77

Hm, then the MCO takes --config-file=/assets/manifests/cluster-config.yaml...is it something like this PR is causing that config to be different only at bootstrap time?

I'm confused too, but I have this pull and openshift/installer#1546 both consistently failing in this mode.

@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Apr 9, 2019

It's most probably the empty Image.config.openshift.io that is triggering https://github.com/openshift/machine-config-operator/tree/master/pkg/controller/container-runtime-config ....

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 9, 2019

New changes are detected. LGTM label has been removed.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 9, 2019

This now only reconciles via the CVO. It relies on openshift/cluster-version-operator#159

@sttts see if you're happy with validation rules.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 9, 2019

/test e2e-aws

kicking since the prereq merged.
@sttts I'd like to see this merged if you're happy with validation.

@deads2k
Copy link
Contributor Author

deads2k commented Apr 9, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

@abhinavdahiya looks like the static pod CVO still has the lock, but I have no logs from the bootstrap CVO. How's that coming?

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

Gaaaaaaah. The mcd mismatch is so frustrating. Surely they should be rendering their initial cluster state or something. This is wrapping us around the axle.

This also implies that something is treating missing and empty differently.

/retest

Just in case

@sttts
Copy link
Contributor

sttts commented Apr 10, 2019

/retest
/hold cancel

@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 Apr 10, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

/retest

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Apr 10, 2019
@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

appears to be openshift/cluster-kube-scheduler-operator#94

@deads2k deads2k mentioned this pull request Apr 10, 2019
Merged
@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

And the console is missing logs.

/retest

@deads2k
Copy link
Contributor Author

deads2k commented Apr 10, 2019

the master pool worked at least once.

/retest

@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 11, 2019

/retest

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

@deads2k
Copy link
Contributor Author

deads2k commented Apr 11, 2019

Looks like it's still having considerable MCD difficulties. I guess I'll try to split out pieces, merge them and find the one the MCD doesn't like.

This was referenced Apr 11, 2019
Merged
Closed
Merged
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 11, 2019

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 11, 2019

/retest

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

@openshift-ci-robot
Copy link

openshift-ci-robot commented Apr 11, 2019

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

Test name Commit Details Rerun command
ci/prow/e2e-aws 01d1f7c link /test e2e-aws

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.

@deads2k deads2k closed this Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@bparees bparees

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@deads2k @bparees @abhinavdahiya @openshift-ci-robot @wking @mfojtik @openshift-bot @sttts @cgwalters