Bug 1794493: add ctrcfg e2e test #1414

haircommander · 2020-01-28T21:23:13Z

- What I did
Added a ctrcfg e2e test, to hopefully begin to prevent regressions
To get this test to pass, some changes were also needed:

refactor some of mcd_test.go (and move to utils_test.go) to not duplicate code between it and ctrcfg_test.go
pull in Bug 1794493: Update crio.yaml templates with correct values #1405, because without it the test fails
revendor cri-o to have an updated config structure
change a check on LogSizeMax which prevents an empty value ending up in the crio.conf

- How to verify it
go test -v -run="TestContainerRuntimeConfigPidsLimit" ./test/e2e/

- Description for the changelog
added simple e2e test for ContainerRuntimeConfigs
revendor cri-o to have an updated config structure

Note: this is WIP because I haven't seen the test pass yet (as I was working without the required patch). Instead of waiting for a cluster to provision manually, I'll now start letting prow do it. I also want to refactor the changes a bit

runcom · 2020-01-28T21:54:54Z

awesome 💪

runcom · 2020-01-28T21:55:29Z

Added a ctrcfg e2e test, to hopefully begin to prevent regressions with it. also pull in #1405, because without it the test fails

I'd just close #1405 and retitle this PR to account for the Bug as well

runcom · 2020-01-29T11:15:46Z

uhm, not sure yet if failures in op tests are fully related

/retest

test/e2e/containerruntimeconfig_test.go

runcom · 2020-01-29T14:47:00Z

uhm I think e2e-gcp-op failures are now related

haircommander · 2020-01-29T15:11:15Z

updated to have the verification the rollback worked, as well as a bit more debugging logs to test the gcp-op problems

runcom · 2020-01-29T18:26:54Z

/skip

haircommander · 2020-01-29T21:33:51Z

/retest

runcom · 2020-01-30T11:21:38Z

/retest

openshift-ci-robot · 2020-01-30T11:23:21Z

@haircommander: This pull request references Bugzilla bug 1794493, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

WIP: Bug 1794493: add ctrcfg e2e test

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.

runcom · 2020-01-30T12:57:00Z

/retest

runcom · 2020-01-30T15:25:20Z

e2e-gcp-op still related to this now that we're back with that

haircommander · 2020-01-30T15:30:39Z

yeah I've observed similar things when testing on my own. somehow the LogSizeMax value is being set to 0, and cri-o complains. Still working on it...

haircommander · 2020-01-30T16:20:19Z


level=error msg="Error: Error applying IAM policy to project \"openshift-gce-devel-ci\": Too many conflicts.  Latest error: Error setting IAM policy for project \"openshift-gce-devel-ci\": googleapi: Error 409: There were concurrent policy changes. Please retry the whole read-modify-write with exponential backoff., aborted"
level=error

/retest

mtrmac

The test LGTM (to the limited extent I understand MCO).

WRT explicitly setting all the CRI-O options:

Neither this nor #1405 provides any rationale. Is there a consensus on this being the right thing?
If it is necessary to explicitly set all of those options, is there a process in place to keep updating the templates as new options are added, or defaults change?

haircommander · 2020-01-30T18:18:32Z

* Neither this nor #1405 provides any rationale. Is there a consensus on this being the right thing?

agreed, I am still working on figuring out all of the failures. Upon this being the final version (still wip), I'll update it for explainations, but as I can see it:
Right now, pkg/controller/container-runtime-config/* defines a custom struct built off of the crio config. This struct is needs to have all of the options populated (for now, because we base off of a single config file). However, if the template doesn't define a default value for a value, that value will be evaluated as empty ("', 0, [], etc) after we render the ctrcfg. The problem is CRI-O can't discern whether that empty value is a mistake or purposeful (for instance, we set stream_address as empty on purpose). That's the first problem that is being fixed.

The second problem is because we are using an outdated version of the CRI-O config, and there are now some values that, when populated, behave better (version file, for instance). So the vendor bump is to fix that.

Finally, there's some weirdness with LogSizeMax. Somehow, it ends up as 0 after an update is applied. I still haven't gotten to the bottom of that, and once I do I'll add a test for each of the options, update the PR description, and remove the WIP

* If it is necessary to explicitly set all of those options, is there a process in place to keep updating the templates as new options are added, or defaults change?

For now, it is necessary and there's no process. However, there's light at the end of the tunnel. What we really should do is utilize drop in config files and define a structure that is a subset of the options that openshift needs (only the ones we care about configuring). When a ctrcfg comes in, we update that file with its values (keeping most out of the config, and thus most defaulting to the system crio.conf)

mtrmac · 2020-01-30T18:53:59Z

* If it is necessary to explicitly set all of those options, is there a process in place to keep updating the templates as new options are added, or defaults change?
For now, it is necessary and there's no process. However, there's light at the end of the tunnel. What we really should do is utilize drop in config files and define a structure that is a subset of the options that openshift needs (only the ones we care about configuring).

That would be great, and is a good reason not to invest too much in the current hardcode-all-default design — if the drop in solution can happen soon enough.

If there’s a risk that this will persist over a CRI-O major release update (1.y to 1.y+1), it seems desirable to harden the MCO implementation, maybe to at build time call https://github.com/cri-o/cri-o/blob/eb14e0feb01971bf88b2591b744b64dd60272fe5/pkg/config/config.go#L485 to generate the templates.

Or maybe do that in updateCRIOConfig: start with the return value of DefaultConfig() (and apply any MCO-specific hard-coded edits in the code) instead of reading and decoding a template file.

haircommander · 2020-02-05T18:41:02Z

 === RUN   TestContainerRuntimeConfigPidsLimit
--- PASS: TestContainerRuntimeConfigPidsLimit (215.71s)

😄
PTAL @kikisdeliveryservice @runcom @umohnani8 @mtrmac

openshift-ci-robot · 2020-02-05T20:26:54Z

@haircommander: This pull request references Bugzilla bug 1794493, which is valid.

Details

In response to this:

Bug 1794493: add ctrcfg e2e test

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.

openshift-ci-robot · 2020-02-05T20:28:45Z

@haircommander: This pull request references Bugzilla bug 1794493, which is valid.

Details

In response to this:

Bug 1794493: add ctrcfg e2e test

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.

openshift-ci-robot · 2020-02-05T20:29:31Z

@haircommander: This pull request references Bugzilla bug 1794493, which is valid.

Details

In response to this:

Bug 1794493: add ctrcfg e2e test

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.

runcom · 2020-02-05T21:18:33Z

there seems to still be some kind of issue/race right? 🤔

haircommander · 2020-02-05T21:24:01Z

there seems to still be some kind of issue/race right?

I don't know if it's an "issue". one could call the sleeps a bit hacky, but I don't know of a better way to make sure the operation has made it to the openshift api-server without them.

runcom · 2020-02-05T22:42:18Z

I don't know if it's an "issue". one could call the sleeps a bit hacky, but I don't know of a better way to make sure the operation has made it to the openshift api-server without them.

oh no sorry, I meant to say the gpg-op test is still failing with something that I think it's related to this change uhm

This reverts commit 69025e8. as well as updates crio.yaml templates with correct values and fixes whitespace Signed-off-by: Urvashi Mohnani <[email protected]> Signed-off-by: Peter Hunt <[email protected]>

Signed-off-by: Peter Hunt <[email protected]>

before, we weren't properly checking LogSizeMax was an empty value. As such, we incorrectly merged the config changes, which bricked the runtime when LogSizeMax wasn't defined. Fix this by using LogSizeMax uniformly by checking against its Value() method Signed-off-by: Peter Hunt <[email protected]>

this change includes: add test/e2e/ctrcfg_test that includes three basic tests for deploying a ctrcfg and seeing it correctly changed the runtime configuration move/refactor some code from test/e2e/mcd_test to test/e2e/utils_test to prevent duplication between the two files, while also being clear about what tests use what (utils currently houses functions used in both mcd and ctrcfg tests) Signed-off-by: Peter Hunt <[email protected]>

haircommander · 2020-02-06T17:00:48Z

Throttling: Rate exceeded\n

/retest

haircommander · 2020-02-06T18:58:34Z

🎉

ok now it's ready (assuming the aws failure is unrelated, which I think it is?)

@runcom @kikisdeliveryservice @umohnani8 @mtrmac PTAL

haircommander · 2020-02-06T19:06:06Z

/retest

haircommander · 2020-02-06T21:42:05Z

/retest

🤞

openshift-ci-robot · 2020-02-06T23:19:24Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`ff58ebb`	link	`/test e2e-aws-scaleup-rhel7`

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.

runcom · 2020-02-07T13:23:37Z

ok now it's ready (assuming the aws failure is unrelated, which I think it is?)

🎉 yep, it looks like it's ok to merge - we'll fix any flake afterwards

runcom · 2020-02-07T13:24:03Z

/skip
/approve
/lgtm

we need a backport for this for 4.3 as well right? @umohnani8 @haircommander

openshift-ci-robot · 2020-02-07T13:26:21Z

[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

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

haircommander · 2020-02-07T13:30:00Z

/skip
/approve
/lgtm

we need a backport for this for 4.3 as well right? @umohnani8 @haircommander

yep, backport here #1447

runcom · 2020-02-10T13:49:12Z

/bugzilla refresh

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 28, 2020

openshift-ci-robot requested review from mtrmac and umohnani8 January 28, 2020 21:24

kikisdeliveryservice self-requested a review January 28, 2020 21:24

kikisdeliveryservice mentioned this pull request Jan 28, 2020

Bug 1794493: Update crio.yaml templates with correct values #1405

Closed

runcom reviewed Jan 29, 2020

View reviewed changes

test/e2e/containerruntimeconfig_test.go Outdated Show resolved Hide resolved

haircommander force-pushed the crio-config-test branch from c636455 to 4d3a597 Compare January 29, 2020 15:08

haircommander force-pushed the crio-config-test branch from 4d3a597 to 276f5e9 Compare January 29, 2020 15:12

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 29, 2020

runcom changed the title ~~WIP: add ctrcfg e2e test~~ WIP: Bug 1794493: add ctrcfg e2e test Jan 30, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 30, 2020

haircommander force-pushed the crio-config-test branch from a9437f3 to b53d27b Compare January 30, 2020 15:46

mtrmac reviewed Jan 30, 2020

View reviewed changes

haircommander force-pushed the crio-config-test branch from 554420c to 96339d9 Compare February 6, 2020 14:02

umohnani8 and others added 4 commits February 6, 2020 11:01

Update and refill templates for CRI-O 1.16

606bd2b

This reverts commit 69025e8. as well as updates crio.yaml templates with correct values and fixes whitespace Signed-off-by: Urvashi Mohnani <[email protected]> Signed-off-by: Peter Hunt <[email protected]>

update to cri-o 1.16.2

ea5d1d4

Signed-off-by: Peter Hunt <[email protected]>

haircommander force-pushed the crio-config-test branch from 96339d9 to ff58ebb Compare February 6, 2020 16:14

haircommander mentioned this pull request Feb 6, 2020

Bug 1794495: [release-4.3] fix ctrcfg and add e2e test #1447

Merged

openshift-ci-robot assigned runcom Feb 7, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2020

openshift-merge-robot merged commit ac44c3f into openshift:master Feb 7, 2020

haircommander deleted the crio-config-test branch February 7, 2020 13:29

wking mentioned this pull request Apr 30, 2020

[release-4.4] crio: Add explicit paths for configuration settings #1700

Closed

Bug 1794493: add ctrcfg e2e test #1414

Bug 1794493: add ctrcfg e2e test #1414

Uh oh!

Conversation

haircommander commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Jan 28, 2020

Uh oh!

runcom commented Jan 28, 2020

Uh oh!

runcom commented Jan 29, 2020

Uh oh!

Uh oh!

runcom commented Jan 29, 2020

Uh oh!

haircommander commented Jan 29, 2020

Uh oh!

runcom commented Jan 29, 2020

Uh oh!

haircommander commented Jan 29, 2020

Uh oh!

runcom commented Jan 30, 2020

Uh oh!

openshift-ci-robot commented Jan 30, 2020

Uh oh!

runcom commented Jan 30, 2020

Uh oh!

runcom commented Jan 30, 2020

Uh oh!

haircommander commented Jan 30, 2020

Uh oh!

haircommander commented Jan 30, 2020

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

haircommander commented Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mtrmac commented Jan 30, 2020

Uh oh!

haircommander commented Feb 5, 2020

Uh oh!

openshift-ci-robot commented Feb 5, 2020

Uh oh!

openshift-ci-robot commented Feb 5, 2020

Uh oh!

openshift-ci-robot commented Feb 5, 2020

Uh oh!

runcom commented Feb 5, 2020

Uh oh!

haircommander commented Feb 5, 2020

Uh oh!

runcom commented Feb 5, 2020

Uh oh!

haircommander commented Feb 6, 2020

Uh oh!

haircommander commented Feb 6, 2020

Uh oh!

haircommander commented Feb 6, 2020

Uh oh!

haircommander commented Feb 6, 2020

Uh oh!

openshift-ci-robot commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Feb 7, 2020

Uh oh!

runcom commented Feb 7, 2020

Uh oh!

openshift-ci-robot commented Feb 7, 2020

Uh oh!

haircommander commented Feb 7, 2020

Uh oh!

runcom commented Feb 10, 2020

Uh oh!

Reviewers

Assignees

Labels

haircommander commented Jan 28, 2020 •

edited

Loading

haircommander commented Jan 30, 2020 •

edited

Loading

openshift-ci-robot commented Feb 6, 2020 •

edited

Loading