From fe3413bb8ebae90f29ce3cc02373f3fc2b5d2fd2 Mon Sep 17 00:00:00 2001
From: jikai <[email protected]>
Date: Mon, 22 Jan 2024 20:19:29 +0800
Subject: [PATCH 08/43] bug fix for device/cgroup/ulimt oci update

Signed-off-by: jikai <[email protected]>
---
 .../executor/container_cb/execution_create.c  |  7 ++-
 src/daemon/modules/api/specs_api.h            |  4 ++
 .../modules/service/service_container.c       | 18 +++---
 src/daemon/modules/spec/specs.c               | 60 +++++++++++++++----
 4 files changed, 63 insertions(+), 26 deletions(-)

diff --git a/src/daemon/executor/container_cb/execution_create.c b/src/daemon/executor/container_cb/execution_create.c
index ca2a9163..e00afb68 100644
--- a/src/daemon/executor/container_cb/execution_create.c
+++ b/src/daemon/executor/container_cb/execution_create.c
@@ -533,12 +533,15 @@ static int merge_config_for_syscontainer(const container_create_request *request
         value = request->rootfs;
     }
 
-    if (append_json_map_string_string(oci_spec->annotations, "rootfs.mount", value)) {
+    // should also update to container spec
+    if (append_json_map_string_string(container_spec->annotations, "rootfs.mount", value)
+        || append_json_map_string_string(oci_spec->annotations, "rootfs.mount", value)) {
         ERROR("Realloc annotations failed");
         ret = -1;
         goto out;
     }
-    if (request->rootfs != NULL && append_json_map_string_string(oci_spec->annotations, "external.rootfs", "true")) {
+    if (request->rootfs != NULL && (append_json_map_string_string(container_spec->annotations, "external.rootfs", "true")
+        || append_json_map_string_string(oci_spec->annotations, "external.rootfs", "true"))) {
         ERROR("Realloc annotations failed");
         ret = -1;
         goto out;
diff --git a/src/daemon/modules/api/specs_api.h b/src/daemon/modules/api/specs_api.h
index f5f6ad8b..f54c0d31 100644
--- a/src/daemon/modules/api/specs_api.h
+++ b/src/daemon/modules/api/specs_api.h
@@ -47,6 +47,10 @@ oci_runtime_spec *load_oci_config(const char *rootpath, const char *name);
 
 oci_runtime_spec *default_spec(bool system_container);
 
+int update_oci_container_cgroups_path(const char *id, oci_runtime_spec *oci_spec, const host_config *host_spec);
+
+int update_oci_ulimit(oci_runtime_spec *oci_spec, const host_config *host_spec);
+
 const oci_runtime_spec *get_readonly_default_oci_spec(bool system_container);
 
 int spec_module_init(void);
diff --git a/src/daemon/modules/service/service_container.c b/src/daemon/modules/service/service_container.c
index 239783b8..a3606a82 100644
--- a/src/daemon/modules/service/service_container.c
+++ b/src/daemon/modules/service/service_container.c
@@ -693,26 +693,21 @@ out:
 
 static int do_oci_spec_update(const char *id, oci_runtime_spec *oci_spec, container_config *container_spec, host_config *hostconfig)
 {
-    __isula_auto_free char *cgroup_parent = NULL;
     int ret;
 
-    // First renew annotations for oci spec, cgroup path, rootfs.mount, native.mask
-    // for iSulad daemon might get updated
+    // Renew annotations for oci spec, cgroup path only,
+    // since lxc uses the "cgroup.dir" in oci annotations to create cgroup
+    // should ensure that container spec has the same annotations as oci spec
     ret = update_spec_annotations(oci_spec, container_spec, hostconfig);
     if (ret < 0) {
         return -1;
     }
 
     // If isulad daemon cgroup parent updated, we should update this config into oci spec
-    cgroup_parent = merge_container_cgroups_path(id, hostconfig);
-    if (cgroup_parent == NULL) {
+    ret = update_oci_container_cgroups_path(id, oci_spec, hostconfig);
+    if (ret < 0) {
         return -1;
     }
-    if (oci_spec->linux->cgroups_path != NULL && strcmp(oci_spec->linux->cgroups_path, cgroup_parent) != 0) {
-        free(oci_spec->linux->cgroups_path);
-        oci_spec->linux->cgroups_path = cgroup_parent;
-        cgroup_parent = NULL;
-    }
 
     // For Linux.Resources, isula update will save changes into oci spec;
     // so we just skip it;
@@ -725,7 +720,8 @@ static int do_oci_spec_update(const char *id, oci_runtime_spec *oci_spec, contai
     }
 
     // If isulad daemon ulimit updated, we should update this config into oci spec.
-    if (merge_global_ulimit(oci_spec) != 0) {
+    ret = update_oci_ulimit(oci_spec, hostconfig);
+    if (ret < 0) {
         return -1;
     }
 
diff --git a/src/daemon/modules/spec/specs.c b/src/daemon/modules/spec/specs.c
index 62e340b1..464b4fb4 100644
--- a/src/daemon/modules/spec/specs.c
+++ b/src/daemon/modules/spec/specs.c
@@ -402,19 +402,8 @@ int update_spec_annotations(oci_runtime_spec *oci_spec, container_config *contai
         return -1;
     }
 
-    /* add rootfs.mount */
-    ret = add_rootfs_mount(container_spec);
-    if (ret != 0) {
-        ERROR("Failed to add rootfs mount");
-        return -1;
-    }
-
-    /* add native.umask */
-    ret = add_native_umask(container_spec);
-    if (ret != 0) {
-        ERROR("Failed to add native umask");
-        return -1;
-    }
+    // other annotations will either not be updated after containers created
+    // or for rootfs mnt and umask, we do not support the update operation
 
     if (merge_annotations(oci_spec, container_spec)) {
         return -1;
@@ -2302,6 +2291,27 @@ char *merge_container_cgroups_path(const char *id, const host_config *host_spec)
     return util_path_join(path, id);
 }
 
+int update_oci_container_cgroups_path(const char *id, oci_runtime_spec *oci_spec, const host_config *hostconfig)
+{
+    if (oci_spec == NULL || oci_spec->linux == NULL) {
+        ERROR("Invalid arguments");
+        return -1;
+    }
+
+    __isula_auto_free char *cgroup_parent = merge_container_cgroups_path(id, hostconfig);
+    if (cgroup_parent == NULL) {
+        return -1;
+    }
+
+    if (oci_spec->linux->cgroups_path != NULL && strcmp(oci_spec->linux->cgroups_path, cgroup_parent) != 0) {
+        free(oci_spec->linux->cgroups_path);
+        oci_spec->linux->cgroups_path = cgroup_parent;
+        cgroup_parent = NULL;
+    }
+
+    return 0;
+}
+
 static int merge_oci_cgroups_path(const char *id, oci_runtime_spec *oci_spec, const host_config *host_spec)
 {
     if (id == NULL || oci_spec == NULL || host_spec == NULL) {
@@ -2445,6 +2455,30 @@ out:
     return ret;
 }
 
+int update_oci_ulimit(oci_runtime_spec *oci_spec, const host_config *hostconfig) {
+    if (oci_spec == NULL || hostconfig == NULL) {
+        ERROR("Invalid arguments");
+        return -1;
+    }
+
+    size_t i = 0;
+    if (oci_spec->process != NULL) {
+        for (i = 0; i < oci_spec->process->rlimits_len; i++) {
+            free_defs_process_rlimits_element(oci_spec->process->rlimits[i]);
+            oci_spec->process->rlimits[i] = NULL;
+        }
+        free(oci_spec->process->rlimits);
+        oci_spec->process->rlimits = NULL;
+        oci_spec->process->rlimits_len = 0;
+    }
+
+    if (merge_conf_ulimits(oci_spec, hostconfig) != 0 || merge_global_ulimit(oci_spec) != 0) {
+        return -1;
+    }
+
+    return 0;
+}
+
 /* read oci config */
 oci_runtime_spec *load_oci_config(const char *rootpath, const char *name)
 {
-- 
2.34.1