Bug 2012838: fix override storage options from storage.conf #5423

QiWang19 · 2021-10-25T20:42:44Z

Fix https://bugzilla.redhat.com/show_bug.cgi?id=2012838

Merge in the storage_option from drop-in crio.conf to the storage.conf, do not override the configs from storage.conf.
The override leads to configuration of containerruntimeconfig not working.

Signed-off-by: Qi Wang qiwan@redhat.com

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

https://bugzilla.redhat.com/show_bug.cgi?id=2012838
The storage_option from /etc/crio/crio.conf.d/00-default overrides the configurations from /etc/containers/storage.conf.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Merge storage_option from drop-in files to sttorage_option from storage.conf

codecov · 2021-10-25T20:46:05Z

Codecov Report

Merging #5423 (8daa903) into main (b5002c4) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #5423      +/-   ##
==========================================
+ Coverage   43.48%   43.52%   +0.04%     
==========================================
  Files         118      118              
  Lines       11848    11864      +16     
==========================================
+ Hits         5152     5164      +12     
- Misses       6204     6207       +3     
- Partials      492      493       +1

haircommander · 2021-10-25T20:47:11Z

I like this, nice and clean

I have two general questions: if there are duplicate entries in the storage options, what happens? Then, if the duplicate entries are handled by choosing the last one (as I assume it does), do you think we should have storage options first, then the crio specific ones? that way, the crio ones will override the default ones, but if they're not defined in crio's config, we'll inherit the storage ones

haircommander · 2021-10-25T20:47:46Z

we could also add a simple test in test/config.bats that calls crio-status config to check the correct values are inherited

TomSweeneyRedHat · 2021-10-27T14:33:29Z

LGTM
assuming happy tests

sonatype-lift · 2021-10-27T19:05:21Z

pkg/config/config.go

+	// keeps the storage options from storage.conf and merge it to crio config
+	var storageOpts []string
+	storageOpts = append(storageOpts, c.StorageOptions...)
+
 	data, err := ioutil.ReadFile(path)


G304: Potential file inclusion via variable
(at-me in a reply with help or ignore)

@sonatype-lift help

I’m LiftBot and I automatically analyze new code in code review, and comment when I find potential bugs. I also accept comments as commands. Just @sonatype-lift followed by the command: ignore to mark as false positive, unignore to undo, and help to see this message. Click here to add LiftBot to another repo.

QiWang19 · 2021-10-27T20:53:31Z

/test critest_fedora

QiWang19 · 2021-10-28T16:26:14Z

@cri-o/cri-o-maintainers PTAL

haircommander · 2021-10-28T16:45:29Z

/approve
/retest

LGTM

QiWang19 · 2021-11-02T14:19:54Z

@cri-o/cri-o-maintainers PTAL

haircommander · 2021-11-02T14:42:40Z

one issue with this approach is when we run
crio -c crio.conf config > crio2.conf && mv crio2.conf crio.conf then I believe we would get an ever growing list. we may want to consider option deduplication

haircommander · 2021-11-02T17:38:36Z

(by that I don't mean checking each key, but each key=value option pair)
another thing to think about is what is the correct behavior here:
["option=value1", "option=value2", "option=value1"]

my intuition would be the resulting list should be
["option=value2", "option=value1"]

QiWang19 · 2021-11-03T17:15:42Z

/test kata-containers

QiWang19 · 2021-11-03T17:24:18Z

Removed the duplicates and keeps the last appearance in the list.

QiWang19 · 2021-11-03T17:24:35Z

@cri-o/cri-o-maintainers PTAL

saschagrunert

/lgtm

openshift-ci · 2021-11-04T07:28:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, QiWang19, saschagrunert

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~~ [haircommander,saschagrunert]

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

openshift-bot · 2021-11-04T07:35:09Z

/retest-required

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

openshift-bot · 2021-11-04T07:59:12Z

/retest-required

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

openshift-bot · 2021-11-04T08:23:11Z

/retest-required

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

openshift-bot · 2021-11-04T08:35:11Z

/retest-required

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

openshift-bot · 2021-11-04T08:47:12Z

/retest-required

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

openshift-bot · 2021-11-04T08:59:11Z

/retest-required

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

openshift-bot · 2021-11-04T09:12:14Z

/retest-required

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

openshift-bot · 2021-11-04T09:24:11Z

/retest-required

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

openshift-bot · 2021-11-04T09:36:10Z

/retest-required

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

openshift-bot · 2021-11-04T09:48:16Z

/retest-required

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

openshift-bot · 2021-11-04T10:13:10Z

/retest-required

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

openshift-ci · 2021-11-04T10:28:40Z

@QiWang19: The following tests 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/openshift-jenkins/integration_crun_cgroupv2	`8daa903`	link	false	`/test integration_cgroupv2`
ci/openshift-jenkins/e2e_crun_cgroupv2	`8daa903`	link	false	`/test e2e_cgroupv2`

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-bot · 2021-11-04T10:38:18Z

/retest-required

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

openshift-bot · 2021-11-04T10:51:12Z

/retest-required

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

openshift-bot · 2021-11-04T11:03:09Z

/retest-required

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

openshift-bot · 2021-11-04T11:40:15Z

/retest-required

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

openshift-bot · 2021-11-04T11:52:11Z

/retest-required

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

openshift-bot · 2021-11-04T12:05:14Z

/retest-required

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

QiWang19 · 2021-11-04T19:46:35Z

/cherry-pick release-1.21

openshift-cherrypick-robot · 2021-11-04T19:47:23Z

@QiWang19: new pull request created: #5436

In response to this:

/cherry-pick release-1.21

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 · 2021-11-04T20:08:28Z

/cherry-pick release-1.22

openshift-cherrypick-robot · 2021-11-04T20:09:07Z

@haircommander: new pull request created: #5437

In response to this:

/cherry-pick release-1.22

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.

QiWang19 requested review from mrunalp and runcom as code owners October 25, 2021 20:42

openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/bug Categorizes issue or PR as related to a bug. labels Oct 25, 2021

openshift-ci bot requested review from haircommander and vrothberg October 25, 2021 20:42

QiWang19 force-pushed the storage-opts branch 3 times, most recently from de6bad3 to 7e2f19f Compare October 27, 2021 02:16

haircommander mentioned this pull request Oct 27, 2021

Bug 2017756: Remove crio settings that overwrite /etc/containers/storage.conf openshift/machine-config-operator#2811

Merged

QiWang19 force-pushed the storage-opts branch from 7e2f19f to cdb1a92 Compare October 27, 2021 18:43

sonatype-lift bot reviewed Oct 27, 2021

View reviewed changes

QiWang19 force-pushed the storage-opts branch 2 times, most recently from dfc897d to 4ec7adc Compare October 27, 2021 19:26

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 28, 2021

QiWang19 force-pushed the storage-opts branch from 4ec7adc to d4a599d Compare November 2, 2021 21:03

saschagrunert approved these changes Nov 4, 2021

View reviewed changes

openshift-ci bot assigned saschagrunert Nov 4, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 4, 2021

openshift-merge-robot merged commit 3943334 into cri-o:main Nov 4, 2021

openshift-cherrypick-robot mentioned this pull request Nov 4, 2021

[release-1.21] Bug 2020724: fix override storage options from storage.conf #5436

Merged

openshift-cherrypick-robot mentioned this pull request Nov 4, 2021

[release-1.22] Bug 2020729: fix override storage options from storage.conf #5437

Merged

QiWang19 mentioned this pull request Feb 14, 2022

REQUEST: New membership for @QiWang19 #5642

Closed

4 tasks

Bug 2012838: fix override storage options from storage.conf #5423

Bug 2012838: fix override storage options from storage.conf #5423

Conversation

QiWang19 commented Oct 25, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

codecov bot commented Oct 25, 2021 • edited

Codecov Report

haircommander commented Oct 25, 2021

haircommander commented Oct 25, 2021

TomSweeneyRedHat commented Oct 27, 2021

sonatype-lift bot Oct 27, 2021

Choose a reason for hiding this comment

QiWang19 Oct 27, 2021

Choose a reason for hiding this comment

sonatype-lift bot Oct 27, 2021

Choose a reason for hiding this comment

QiWang19 commented Oct 27, 2021

QiWang19 commented Oct 28, 2021

haircommander commented Oct 28, 2021

QiWang19 commented Nov 2, 2021

haircommander commented Nov 2, 2021

haircommander commented Nov 2, 2021

QiWang19 commented Nov 3, 2021

QiWang19 commented Nov 3, 2021

QiWang19 commented Nov 3, 2021

saschagrunert left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-ci bot commented Nov 4, 2021 • edited

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

openshift-bot commented Nov 4, 2021

QiWang19 commented Nov 4, 2021

openshift-cherrypick-robot commented Nov 4, 2021

haircommander commented Nov 4, 2021

openshift-cherrypick-robot commented Nov 4, 2021

codecov bot commented Oct 25, 2021 •

edited

openshift-ci bot commented Nov 4, 2021 •

edited