-
Notifications
You must be signed in to change notification settings - Fork 255
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
make container's shared memory configurable via annotation #1052
Conversation
a26e6e5
to
f258981
Compare
Just out of curiosity: Why do we do this in gcs than doing it during container document creation? |
@ambarve I think I'd said that we handle a similar annotation that changes the oci spec (io.microsoft.virtualmachine.lcow.privileged) in the guest so it might make sense to do the same for this. Maybe we should move both out to the initial document creation if it makes sense |
Why not filter out the initial mount in CRI the way /dev and /sys/fs/cgroup mounts are filtered out today? |
all the way to CRI? or? |
I started off by doing something like this: anmaxvl/cri@cb87112, which needs to be changed since we'll eventually have 2 configs: 1 for sandbox /dev/shm (which needs to be implemented first) and the second one is for container (current PR). |
same question as to @dcantah. |
No just in the LCOW container document setup in the shim I was thinking. I feel like checking/having logic for our annotations in our cri fork is gonna be a pain if we ever move off of it (but I'm guilty of doing the same). |
looking a bit more into it, we could update the |
@anmaxvl I don't mind it here, don't know about others. |
I don't really have an issue with it being done in the gcs but maybe we should have a function that sets up all of these annotation specific overrides instead of them being inline, as there's now three. e.g. func <validateAnnotationsOrSomething>(spec *oci.Spec) error {
if val, ok := spec.Annotations["io.microsoft.container.storage.shm.size-kb"]; ok {
sz, err := strconv.ParseInt(val, 10, 64)
if err != nil {
return errors.Wrap(err, "/dev/shm size must be a valid integer")
}
if sz <= 0 {
return errors.Errorf("/dev/shm size must be a positive integer, got: %d", sz)
}
size := fmt.Sprintf("size=%dk", sz)
mt := oci.Mount{
Destination: "/dev/shm",
Type: "tmpfs",
Source: "shm",
Options: []string{"nosuid", "noexec", "nodev", "mode=1777", size},
}
spec.Mounts = removeMount("/dev/shm", spec.Mounts)
spec.Mounts = append(spec.Mounts, mt)
}
// Check if we need to do any capability/device mappings
if spec.Annotations["io.microsoft.virtualmachine.lcow.privileged"] == "true" {
log.G(ctx).Debug("'io.microsoft.virtualmachine.lcow.privileged' set for privileged container")
// Add all host devices
hostDevices, err := devices.HostDevices()
if err != nil {
return err
}
for _, hostDevice := range hostDevices {
addLinuxDeviceToSpec(ctx, hostDevice, spec, false)
}
// Set the cgroup access
spec.Linux.Resources.Devices = []oci.LinuxDeviceCgroup{
{
Allow: true,
Access: "rwm",
},
}
} else {
tempLinuxDevices := spec.Linux.Devices
spec.Linux.Devices = []oci.LinuxDevice{}
for _, ld := range tempLinuxDevices {
hostDevice, err := devices.DeviceFromPath(ld.Path, "rwm")
if err != nil {
return err
}
addLinuxDeviceToSpec(ctx, hostDevice, spec, true)
}
}
if userstr, ok := spec.Annotations["io.microsoft.lcow.userstr"]; ok {
if err := setUserStr(spec, userstr); err != nil {
return err
}
}
return nil
} |
f258981
to
652fc5b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. Would like to see what others think about altering the spec in the guest from annotations though.
652fc5b
to
3aa927b
Compare
3aa927b
to
019a2da
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this LGTM!
I am okay with keeping the annotation processing part in opengcs since moving the other two annotations on hcsshim side won't work.
add annotation "io.microsoft.container.storage.shm.size-kb" to set container's /dev/shm tmpfs size. this overrides any existing /dev/shm mounts in the spec additionally move the annotations parsing logic into a separate function Signed-off-by: Maksim An <[email protected]>
019a2da
to
461cf9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Related work items: microsoft#930, microsoft#962, microsoft#1004, microsoft#1008, microsoft#1039, microsoft#1045, microsoft#1046, microsoft#1047, microsoft#1052, microsoft#1053, microsoft#1054, microsoft#1057, microsoft#1058, microsoft#1060, microsoft#1061, microsoft#1063, microsoft#1064, microsoft#1068, microsoft#1069, microsoft#1070, microsoft#1071, microsoft#1074, microsoft#1078, microsoft#1079, microsoft#1081, microsoft#1082, microsoft#1083, microsoft#1084, microsoft#1088, microsoft#1090, microsoft#1091, microsoft#1093, microsoft#1094, microsoft#1096, microsoft#1098, microsoft#1099, microsoft#1102, microsoft#1103, microsoft#1105, microsoft#1106, microsoft#1108, microsoft#1109, microsoft#1115, microsoft#1116, microsoft#1122, microsoft#1123, microsoft#1126
This PR adds annotation
"io.microsoft.container.storage.shm.size-kb"
toset container's
/dev/shm
tmpfs size, which overrides any existing/dev/shm
mount in the spec
Signed-off-by: Maksim An [email protected]