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

Add DefaultContainerAnnotations runhcs option #1210

Merged

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Oct 28, 2021

Add default_container_annotations to runhcs options protos

Set container spec Annotations from runhcs options

This PR adds support for a scenario, when we'd want to set default
annotations on all containers within a deployment, e.g. /dev/shm size,
container's scratch size or any other setting controlled by annotations.
The default annotations don't override the ones passed explicitly in the
container spec.

@anmaxvl anmaxvl requested a review from a team as a code owner October 28, 2021 22:17
@anmaxvl anmaxvl force-pushed the lcow-runhcs-container-annotations-option branch from e6212c5 to 3784904 Compare October 28, 2021 22:19
@anmaxvl anmaxvl changed the title Lcow runhcs container annotations option LCOW runhcs container annotations option Oct 28, 2021
@anmaxvl anmaxvl changed the title LCOW runhcs container annotations option Add GlobalContainerAnnotations runhcs option Oct 28, 2021
@anmaxvl
Copy link
Contributor Author

anmaxvl commented Nov 1, 2021

Assigning to @kevpar, since it was his idea 😄

Signed-off-by: Maksim An <maksiman@microsoft.com>
Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl force-pushed the lcow-runhcs-container-annotations-option branch from 3784904 to c5f2a39 Compare November 1, 2021 22:26
@ambarve
Copy link
Contributor

ambarve commented Nov 9, 2021

@anmaxvl Could you please add a description about why this is required?

Signed-off-by: Maksim An <maksiman@microsoft.com>
@anmaxvl anmaxvl changed the title Add GlobalContainerAnnotations runhcs option Add DefaultContainerAnnotations runhcs option Nov 10, 2021
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -395,5 +395,11 @@ func UpdateSpecFromOptions(s specs.Spec, opts *runhcsopts.Options) specs.Spec {
s.Annotations[annotations.NetworkConfigProxy] = opts.NCProxyAddr
}

for key, value := range opts.DefaultContainerAnnotations {
Copy link
Member

Choose a reason for hiding this comment

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

It's weird this code lives in uvm.go, given it is not at all specific to UVMs, or hv-isolation. Perhaps we can address that in a future change.

Copy link
Member

@kevpar kevpar left a comment

Choose a reason for hiding this comment

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

LGTM

@anmaxvl anmaxvl merged commit 0417366 into microsoft:master Nov 10, 2021
@anmaxvl anmaxvl deleted the lcow-runhcs-container-annotations-option branch November 10, 2021 23:34
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
protos: Add default_container_annotations to runhcs options protos

Assign default container annotations from runhcs options to
container spec Annotations, without overriding the ones that are
explicitly passed

Signed-off-by: Maksim An <maksiman@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants