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.

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

config: add default_annotations #8829

Merged
merged 1 commit into from
Dec 18, 2024
Merged

config: add default_annotations #8829

merged 1 commit into from
Dec 18, 2024

Conversation

haircommander
Copy link
Member

What type of PR is this?

What this PR does / why we need it:

/kind feature

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

add `default_annotations` to the runtime handler configuration field, allowing admins to specify default annotations to pass to pods

@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Dec 9, 2024
@openshift-ci openshift-ci bot requested review from hasan4791 and klihub December 9, 2024 21:08
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 9, 2024
@haircommander haircommander force-pushed the default_annotations branch 3 times, most recently from d042140 to f3bc919 Compare December 9, 2024 21:35
@codecov Codecov
Copy link

codecov bot commented Dec 9, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 22.72727% with 17 lines in your changes missing coverage. Please review.

Project coverage is 47.10%. Comparing base (8564f35) to head (a103688).
Report is 6 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #8829      +/-   ##
==========================================
+ Coverage   46.95%   47.10%   +0.14%     
==========================================
  Files         154      154              
  Lines       21978    22113     +135     
==========================================
+ Hits        10320    10416      +96     
- Misses      10599    10631      +32     
- Partials     1059     1066       +7

kwilczynski
kwilczynski reviewed Dec 10, 2024
View reviewed changes
pkg/config/template.go Outdated Show resolved Hide resolved
@haircommander haircommander force-pushed the default_annotations branch 6 times, most recently from ba3da90 to fd14682 Compare December 16, 2024 16:03
@sohankunkerkar
Copy link
Member

You need to update https://github.com/cri-o/cri-o/blob/main/docs/crio.conf.5.md#crioruntimeruntimes-table

sohankunkerkar
sohankunkerkar reviewed Dec 16, 2024
View reviewed changes
pkg/config/template.go
@@ -1271,6 +1272,7 @@ const templateStringCrioRuntimeRuntimesRuntimeHandler = `# The "crio.runtime.run
# - no_sync_log (optional, bool): If set to true, the runtime will not sync the log file on rotate or container exit.
# This option is only valid for the 'oci' runtime type. Setting this option to true can cause data loss, e.g.
# when a machine crash happens.
# - default_annotations (optional, map): Default annotations if not overridden by the pod spec.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to add this for default_annotations.

@haircommander
Copy link
Member Author

thanks @sohankunkerkar updated

@haircommander haircommander force-pushed the default_annotations branch 2 times, most recently from c272dcc to 15abbb0 Compare December 16, 2024 17:38
@kwilczynski
Copy link
Member

/hold

To allow others to review.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2024
@kwilczynski
Copy link
Member

/approve
/lgtm

@kwilczynski
Copy link
Member

/retest

2 similar comments
@saschagrunert
Copy link
Member

/retest

@sohankunkerkar
Copy link
Member

/retest

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2024
@haircommander haircommander added this to the 1.32 milestone Dec 17, 2024
@haircommander
Copy link
Member Author

/retest

1 similar comment
@saschagrunert
Copy link
Member

/retest

@saschagrunert
Copy link
Member

/override ci/prow/e2e-gcp-ovn
/override ci/prow/ci-fedora-critest

@saschagrunert
Copy link
Member

/test ci-fedora-kata

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/ci-fedora-critest, ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-gcp-ovn
/override ci/prow/ci-fedora-critest

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-sigs/prow repository.

@saschagrunert
Copy link
Member

/override ci/prow/e2e-aws-ovn

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/e2e-aws-ovn

In response to this:

/override ci/prow/e2e-aws-ovn

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-sigs/prow repository.

saschagrunert
saschagrunert requested changes Dec 18, 2024
View reviewed changes
test/pod.bats
Comment on lines +537 to +557
@test "run container with default annotations" {
setup_crio

cat << EOF > "$CRIO_CONFIG_DIR/99-ann.conf"
[crio.runtime]
default_runtime = "ann"
[crio.runtime.runtimes.ann]
runtime_path = "$RUNTIME_BINARY_PATH"
default_annotations = { "hello" = "1234", "pod" = "5678" }
EOF
unset CONTAINER_DEFAULT_RUNTIME
unset CONTAINER_RUNTIMES

start_crio_no_setup

ctr_id=$(crictl run "$TESTDATA"/container_sleep.json "$TESTDATA"/sandbox_config.json)
annotations=$(crictl inspect "$ctr_id" | jq .info.runtimeSpec.annotations)
grep hello <<< "$annotations"
# pod spec should override default annotations
grep -v "5678" <<< "$annotations"
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to exclude this test or make it work in kata:

not ok 135 run container with default annotations
# (in test file pod.bats, line 564)
#   `ctr_id=$(crictl run "$TESTDATA"/container_sleep.json "$TESTDATA"/sandbox_config.json)' failed
# time="2024-12-18T10:47:50.090479464Z" level=info msg="Updating config from single file: /etc/crio/crio.conf"
# time="2024-12-18T10:47:50.090505488Z" level=info msg="Updating config from drop-in file: /etc/crio/crio.conf"
# time="2024-12-18T10:47:50.0913983Z" level=info msg="Updating config from path: /etc/crio/crio.conf.d"
# time="2024-12-18T10:47:50.091518626Z" level=info msg="Updating config from drop-in file: /etc/crio/crio.conf.d/01-ns-lifecycle.conf"
# time="2024-12-18T10:47:50.091604066Z" level=info msg="Updating config from drop-in file: /etc/crio/crio.conf.d/01-overlay.conf"
# time="2024-12-18T10:47:50.091644374Z" level=info msg="Updating config from drop-in file: /etc/crio/crio.conf.d/99-log-level.conf"
# level=info msg="Using default capabilities: CAP_CHOWN, CAP_DAC_OVERRIDE, CAP_FSETID, CAP_FOWNER, CAP_SETGID, CAP_SETUID, CAP_SETPCAP, CAP_NET_BIND_SERVICE, CAP_KILL"
# time="2024-12-18T10:47:50.129665841Z" level=info msg="Updating config from path: "
# level=info msg="Using default capabilities: CAP_CHOWN, CAP_DAC_OVERRIDE, CAP_FSETID, CAP_FOWNER, CAP_SETGID, CAP_SETUID, CAP_SETPCAP, CAP_NET_BIND_SERVICE, CAP_KILL"
# time="2024-12-18T10:47:50Z" level=fatal msg="validate service connection: validate CRI v1 runtime API for endpoint \"unix:///tmp/tmp.lvCNqQ86CV/crio.sock\": rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial unix /tmp/tmp.lvCNqQ86CV/crio.sock: connect: no such file or directory\""
# E1218 10:47:51.470463   62497 log.go:32] "CreateContainer in sandbox from runtime service failed" err=<
# 	rpc error: code = Unknown desc = container create failed: flag provided but not defined: -systemd-cgroup
# 	Usage of /opt/kata/bin/containerd-shim-kata-v2:
# 	  -address string
# 	    	grpc address back to main containerd
# 	  -bundle string
# 	    	path to the bundle if not workdir
# 	  -debug
# 	    	enable debug output in logs
# 	  -id string
# 	    	id of the task
# 	  -namespace string
# 	    	namespace that owns the shim
# 	  -publish-binary string
# 	    	path to publish binary (used for publishing events), but /opt/kata/bin/containerd-shim-kata-v2 will ignore this flag, please use the TTRPC_ADDRESS env
# 	  -socket string
# 	    	socket path to serve
# 	  -v	show the shim version and exit
#  > podSandboxID="361574031e833c28e96e692b540c180d83c3c0090173c17c158de79f8fade5dc"
# time="2024-12-18T10:47:51Z" level=fatal msg="running container: creating container failed: rpc error: code = Unknown desc = container create failed: flag provided but not defined: -systemd-cgroup\nUsage of /opt/kata/bin/containerd-shim-kata-v2:\n  -address string\n    \tgrpc address back to main containerd\n  -bundle string\n    \tpath to the bundle if not workdir\n  -debug\n    \tenable debug output in logs\n  -id string\n    \tid of the task\n  -namespace string\n    \tnamespace that owns the shim\n  -publish-binary string\n    \tpath to publish binary (used for publishing events), but /opt/kata/bin/containerd-shim-kata-v2 will ignore this flag, please use the TTRPC_ADDRESS env\n  -socket string\n    \tsocket path to serve\n  -v\tshow the shim version and exit\n"

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/cri-o_cri-o/8829/pull-ci-cri-o-cri-o-main-ci-fedora-kata/1869327673696718848/artifacts/fedora-kata/cri-o-gather/artifacts/testout.txt

@haircommander
config: add default_annotations
a103688 
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kwilczynski, 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

@saschagrunert
Copy link
Member

/override ci/prow/e2e-aws-ovn
/override ci/prow/e2e-gcp-ovn

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

@saschagrunert: Overrode contexts on behalf of saschagrunert: ci/prow/e2e-aws-ovn, ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-aws-ovn
/override ci/prow/e2e-gcp-ovn

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-sigs/prow repository.

@haircommander
Copy link
Member Author

/override ci/prow/e2e-gcp-ovn

1 similar comment
@haircommander
Copy link
Member Author

/override ci/prow/e2e-gcp-ovn

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

@haircommander: Overrode contexts on behalf of haircommander: ci/prow/e2e-gcp-ovn

In response to this:

/override ci/prow/e2e-gcp-ovn

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-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 4766e09 into cri-o:main Dec 18, 2024
64 of 70 checks passed
@haircommander
Copy link
Member Author

/cherry-pick release-1.31

@openshift-cherrypick-robot
Copy link

@haircommander: new pull request created: #8862

In response to this:

/cherry-pick release-1.31

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-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Dec 18, 2024
Merged
@github-actions github-actions bot mentioned this pull request Dec 22, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sohankunkerkar sohankunkerkar sohankunkerkar left review comments

@kwilczynski kwilczynski kwilczynski left review comments

@saschagrunert saschagrunert saschagrunert approved these changes

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@klihub klihub Awaiting requested review from klihub

Assignees

@kwilczynski kwilczynski

@saschagrunert saschagrunert

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes.
Projects
None yet
Milestone
1.32
Development

Successfully merging this pull request may close these issues.
5 participants
@haircommander @sohankunkerkar @kwilczynski @saschagrunert @openshift-cherrypick-robot