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

Update templates for CRI-O 1.14 #858

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@haircommander
Copy link

commented Jun 14, 2019

- What I did
Updated the crio.yaml templates to the current configuration
- How to verify it
./bin/crio --config $OLD_TEMPLATE > $NEW_TEMPLATE
then replace file_locking{,_path} defaults, as file locking isn't desired (these values were in the wrong place before, so they weren't effecting the config despite being set)
- Description for the changelog
Update crio.yaml templates to match CRI-O 1.14.x config format

@haircommander

This comment has been minimized.

Copy link
Author

commented Jun 14, 2019

I think we also want this in release-4.2, I can open a PR if needed, but I figured we could review/get it ready here before doing the backport.
cc: @umohnani8 @mrunalp

@umohnani8

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

LGTM, thanks @haircommander!

#registries = [
# ]
# "docker.io",

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jun 14, 2019

Member

nit: uneven indentation

This comment has been minimized.

Copy link
@haircommander

haircommander Jun 14, 2019

Author

ah good catch, fixed

#registries = [
# ]
# "docker.io",

This comment has been minimized.

Copy link
@ashcrow

ashcrow Jun 14, 2019

Member

nit: uneven indentation

This comment has been minimized.

Copy link
@haircommander

haircommander Jun 14, 2019

Author

also fixed here

@ashcrow
Copy link
Member

left a comment

2 nits, 1 question. I'm not an expert on the cri-o configuration options but it seems sane and mostly documentation.

@haircommander haircommander force-pushed the haircommander:template-update branch from 8cc0b98 to 3057263 Jun 14, 2019

@kikisdeliveryservice kikisdeliveryservice requested review from runcom and removed request for jlebon Jun 14, 2019

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

I think we also want this in release-4.2, I can open a PR if needed, but I figured we could review/get it ready here before doing the backport.

No backports needed. 😄

@runcom

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@umohnani8 @haircommander do we need to wait for that particular crio version to first land in rhcos before merging this?

/approve
/retest

@openshift-ci-robot

This comment has been minimized.

Copy link

commented Jun 15, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, runcom

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:

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

@haircommander

This comment has been minimized.

Copy link
Author

commented Jun 15, 2019

@runcom yeah the new format isn't backwards compatible. CRI-O should be rolled out in the next week and I'll revisit this once it is.

@runcom

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@runcom yeah the new format isn't backwards compatible. CRI-O should be rolled out in the next week and I'll revisit this once it is.

thanks, I guess that it's fine as long as we don't break upgrades. Holding till we have the new CRI-O

/hold

@haircommander

This comment has been minimized.

Copy link
Author

commented Jun 27, 2019

1.14.5 is in the puddle, let's try again
/retest
/hold cancel

@haircommander haircommander changed the title Update templates for CRI-O 1.14.4 Update templates for CRI-O 1.14 Jun 27, 2019

@kikisdeliveryservice

This comment has been minimized.

Copy link
Member

commented Jun 27, 2019

@runcom this hit the same bug as my PR as well! (BZ 1724346)

ftr see in tests logs e2e-aws: artifacts/e2e-aws/installer/logbundle..../bootstrap/journals/kubelet.log :
Observed a panic: "invalid memory address or nil pointer dereference" (runtime error: invalid memory address or nil pointer

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 2, 2019

/retest

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 3, 2019

/test e2e-aws

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 4, 2019

Happy tests @runcom PTAL

@haircommander haircommander force-pushed the haircommander:template-update branch from 3057263 to ec1d767 Jul 5, 2019

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 6, 2019

/retest

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 8, 2019

/test e2e-aws

@haircommander haircommander force-pushed the haircommander:template-update branch from ec1d767 to 6f101eb Jul 10, 2019

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 10, 2019

/retest

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 10, 2019

ah, shoot, @runcom you were right, it messes with upgrades https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade-4.1/100

I may update with something that will work for 4.3, but in the meantime

/close

@openshift-ci-robot

This comment has been minimized.

Copy link

commented Jul 10, 2019

@haircommander: Closed this PR.

In response to this:

ah, shoot, @runcom you were right, it messes with upgrades https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade-4.1/100

I may update with something that will work for 4.3, but in the meantime

/close

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.

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 10, 2019

/reopen
I lied, I updated it so it is backwards compatible but still updates a couple of things. We can go for the plugin_dirs change in 4.3. I'll test the upgrade once I have the ci image

@openshift-ci-robot

This comment has been minimized.

Copy link

commented Jul 10, 2019

@haircommander: Reopened this PR.

In response to this:

/reopen
I lied, I updated it so it is backwards compatible but still updates a couple of things. We can go for the plugin_dirs change in 4.3. I'll test the upgrade once I have the ci image

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.

@cgwalters

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

To handle upgrades like this I think we'd need to have the renderer verify that the target osImageURL is new enough..though honestly a mess this gets into is that we're rewriting the configuration for the current deployment so it's not really transactional...

Probably indeed the simplest is to land incompatible config changes only once we know that OS content we're upgrading from is compatible, so drop 4.1 cruft in 4.3 as you say, relying there being no direct Cincinnati edges from 4.1 ➡️ 4.3.

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 10, 2019

@cgwalters my original hope was the config changes would roll over after crio had been upgraded, and opened the PR with that assumption. Given that is not necessarily the case, I see no problem updating the config a release later

@haircommander haircommander force-pushed the haircommander:template-update branch from 11bb118 to 2308740 Jul 10, 2019

@runcom

This comment has been minimized.

Copy link
Member

commented Jul 11, 2019

I lied, I updated it so it is backwards compatible but still updates a couple of things. We can go for the plugin_dirs change in 4.3. I'll test the upgrade once I have the ci image

isn't there no way to make these crio config changes backward compatible?

@haircommander

This comment has been minimized.

Copy link
Author

commented Jul 11, 2019

@runcom The one change that breaks backwards compatibility was reverted (plugin_dirs ->plugin_dir). everything else is mostly a slight rearrangement/update to the config version for 1.13.

I tried this update to test an upgrade from 4.1.4, but something is wrong with the way I'm testing it (cluster bot), as it fails along with another PR from this repo that seemed inoffensive (#947) I will keep trying the upgrade, but this PR should not offend crio 1.13

/retest

@haircommander haircommander force-pushed the haircommander:template-update branch from 2308740 to d9e07ee Jul 11, 2019

haircommander added some commits Jul 5, 2019

Update templates for CRI-O 1.14.4
The crio templates included are pretty old. Update them with the same values as before.

Signed-off-by: Peter Hunt <pehunt@redhat.com>
switch to plugin_dir to not break upgrade
Now we will wait to 4.3 to add plugin_dirs

Signed-off-by: Peter Hunt <pehunt@redhat.com>

@haircommander haircommander force-pushed the haircommander:template-update branch from d9e07ee to a7169b7 Jul 11, 2019

call make update to fix CI
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@openshift-ci-robot

This comment has been minimized.

Copy link

commented Jul 11, 2019

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

Test name Commit Details Rerun command
ci/prow/verify a651f6f link /test verify
ci/prow/e2e-aws-op a651f6f link /test e2e-aws-op
ci/prow/e2e-aws a651f6f link /test e2e-aws
ci/prow/e2e-aws-upgrade a651f6f link /test e2e-aws-upgrade

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-ci-robot

This comment has been minimized.

Copy link

commented Jul 12, 2019

@haircommander: PR needs rebase.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.