gha: Setup nydus snapshotter for CoCo tests #8585

ChengyuZhu6 · 2023-12-06T06:39:24Z

Setup nydus snapshotter for CoCo tests.

Fixes #8584

stevenhorsman

Looks good, just a few questions/comments

tests/common.bash

tests/gha-run-k8s-common.sh

fidencio · 2023-12-06T22:53:50Z

In order to make things consistent for both Kata Containers and Confidential Containers, I wonder if we work with the nydus community to have a payload coming from them, that could be used both here and by the Confidential Containers CI.

This would reduce a lot the chances of misconfigurations, and that would be a "trusted" source that both projects could consume.

IOW, we'd provide them the patches, and then consume their payload here and in CoCo.

WDYT?

ChengyuZhu6 · 2023-12-07T02:39:48Z

In order to make things consistent for both Kata Containers and Confidential Containers, I wonder if we work with the nydus community to have a payload coming from them, that could be used both here and by the Confidential Containers CI.

This would reduce a lot the chances of misconfigurations, and that would be a "trusted" source that both projects could consume.

IOW, we'd provide them the patches, and then consume their payload here and in CoCo.

WDYT?

I think that's a great idea.

ChengyuZhu6 · 2023-12-07T03:29:52Z

@stevenhorsman @fidencio The PR has been merged.

ChengyuZhu6 · 2023-12-07T06:35:46Z

@stevenhorsman @fidencio Should we support an official release of the Nydus snapshotter for arm64?

liudalibj

there are some typo

.github/workflows/run-kata-coco-tests.yaml

stevenhorsman · 2023-12-07T10:43:54Z

@stevenhorsman @fidencio Should we support an official release of the Nydus snapshotter for arm64?

I think it would be worth starting on this as in main we produce arm64 payloads, even if we don't have any CCA runtime, or tests yet AFAIK. I don't think it should block this PR though

tests/gha-run-k8s-common.sh

ChengyuZhu6 · 2024-02-07T12:34:52Z

The failures about run-nydus are caused by the new release name, which differs from the version used by kata now. The PR is to resolve the issue.

ChengyuZhu6 · 2024-02-08T10:24:57Z

/test

ChengyuZhu6 · 2024-02-08T10:25:36Z

@fidencio @wainersm @stevenhorsman I found that the snapshotter can be deployed and cleaned up as expected. This PR does not configure the snapshotter in containerd, so it should not affect the existing tests. However, I encountered some failures that I could not explain. Could you please help me troubleshoot them? If the tests pass, can we merge the PR first?

fidencio

I've left a few comments that I will address as Chengyu is on holidays.

fidencio · 2024-02-09T13:39:46Z

tests/integration/kubernetes/gha-run.sh

@@ -21,7 +21,8 @@ DOCKER_REGISTRY=${DOCKER_REGISTRY:-quay.io}
 DOCKER_REPO=${DOCKER_REPO:-kata-containers/kata-deploy-ci}
 DOCKER_TAG=${DOCKER_TAG:-kata-containers-latest}
 KATA_DEPLOY_WAIT_TIMEOUT=${KATA_DEPLOY_WAIT_TIMEOUT:-10m}
-KATA_HYPERVISOR=${KATA_HYPERVISOR:-qemu}
+SNAPSHOTTER_DEPLOY_WAIT_TIMEOUT=${SNAPSHOTTER_DEPLOY_WAIT_TIMEOUT:-5m}
+KATA_HYPERVISOR=${KATA_HYPERVISOR-qemu}


Suggested change

KATA_HYPERVISOR=${KATA_HYPERVISOR-qemu}

KATA_HYPERVISOR=${KATA_HYPERVISOR:-qemu}

fidencio · 2024-02-09T13:40:22Z

tests/integration/kubernetes/gha-run.sh

 	echo "::endgroup::"
 }

 function cleanup_snapshotter() {
 	echo "::group::Cleanuping ${SNAPSHOTTER:-}"
-	#TODO Add the logic for cleaning up the snapshotter in PR https://github.com/kata-containers/kata-containers/pull/8585.
+	case ${SNAPSHOTTER:-} in


I'd prefer having SNAPSHOTTER declared atop, instead of having SNAPSHOTTER:- everywhere.

fidencio · 2024-02-09T13:41:50Z

tests/integration/kubernetes/gha-run.sh

+	sleep 180s
+	kubectl delete -f "misc/snapshotter/nydus-snapshotter-rbac.yaml"
+	popd
+	sleep 180s


rbac deletion usually is quite fast, I'd drop this to 30s or so, instead of 3 minutes.

fidencio · 2024-02-09T13:42:18Z

tests/integration/kubernetes/gha-run.sh

@@ -21,7 +21,8 @@ DOCKER_REGISTRY=${DOCKER_REGISTRY:-quay.io}
 DOCKER_REPO=${DOCKER_REPO:-kata-containers/kata-deploy-ci}
 DOCKER_TAG=${DOCKER_TAG:-kata-containers-latest}
 KATA_DEPLOY_WAIT_TIMEOUT=${KATA_DEPLOY_WAIT_TIMEOUT:-10m}
-KATA_HYPERVISOR=${KATA_HYPERVISOR:-qemu}
+SNAPSHOTTER_DEPLOY_WAIT_TIMEOUT=${SNAPSHOTTER_DEPLOY_WAIT_TIMEOUT:-5m}


5m seems like way too much as well, maybe 3 minutes is okay.

Let's leave this as 5 minutes for now, as I just realised that the kata-deploy timeout is 10 minutes.

We can use daemonset to deploy nydus snapshotter, which will decrease one manual step both for Kata Containers and Confidential Containers CI. Fixes: kata-containers#8584 Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

Cleanup nydus snapshotter by the daemonset. Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

fidencio

lgtm, thanks @ChengyuZhu6!

fidencio · 2024-02-09T13:48:07Z

/test

gkurz

I still cannot tell from a nydus standpoint but LGTM from a pure scripting point of view.

Thanks @fidencio and @ChengyuZhu6 !

stevenhorsman

LGTM. Thanks @ChengyuZhu6 and reviewers!

stevenhorsman · 2024-02-09T14:02:56Z

tests/integration/kubernetes/gha-run.sh

+	git clone -b "${nydus_snapshotter_version}" "${nydus_snapshotter_url}" "${nydus_snapshotter_install_dir}"
+
+	pushd "$nydus_snapshotter_install_dir"
+	if [ "${PULL_TYPE}" == "guest-pull" ]; then


Not a review comment, just to remind that PULL_TYPE is set to guest-pull in the pre-PR to this that updates the workflows: a214bd8

This PR is a split of kata-containers#8585. make the changes on the Github workflows, and the skeleton to deploy_snapshotter() and cleanup_snapshotter() in tests/integration/kubernetes/gha-run.sh in this commit. After initially merging this patch to trigger CI jobs for CoCo, which will begin executing the dummy functions deploy_snapshotter() and cleanup_snapshotter(), the implementation details for these functions remain in kata-containers#8585. Our subsequent step involves transferring this logic to the PR kata-containers#8484, enabling the PR to undergo CI testing prior to its merge. Fixes: kata-containers#8997 Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

katacontainersbot added the size/large Task of significant size label Dec 6, 2023

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch from ad511c4 to 6e15249 Compare December 6, 2023 06:40

ChengyuZhu6 requested review from fidencio, stevenhorsman, jiangliu and wainersm December 6, 2023 06:41

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch 6 times, most recently from c620fac to 3800892 Compare December 6, 2023 07:06

ChengyuZhu6 mentioned this pull request Dec 6, 2023

Merge basic guest pull image code to main #8484

Merged

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch from 3800892 to 715067b Compare December 6, 2023 08:54

ChengyuZhu6 added the no-backport-needed label Dec 6, 2023

stevenhorsman reviewed Dec 6, 2023

View reviewed changes

tests/common.bash Outdated Show resolved Hide resolved

tests/gha-run-k8s-common.sh Outdated Show resolved Hide resolved

tests/gha-run-k8s-common.sh Outdated Show resolved Hide resolved

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch 4 times, most recently from d1e05a1 to 23f135b Compare December 7, 2023 06:50

liudalibj suggested changes Dec 7, 2023

View reviewed changes

.github/workflows/run-kata-coco-tests.yaml Outdated Show resolved Hide resolved

.github/workflows/run-kata-coco-tests.yaml Outdated Show resolved Hide resolved

.github/workflows/run-kata-coco-tests.yaml Outdated Show resolved Hide resolved

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch from f0c1082 to 16b8552 Compare December 7, 2023 08:07

ChengyuZhu6 requested review from liudalibj and stevenhorsman December 7, 2023 10:35

stevenhorsman reviewed Dec 7, 2023

View reviewed changes

tests/gha-run-k8s-common.sh Outdated Show resolved Hide resolved

ChengyuZhu6 removed the ok-to-test label Feb 7, 2024

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch from f243450 to 986555e Compare February 7, 2024 14:49

ChengyuZhu6 added the ok-to-test label Feb 8, 2024

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch 5 times, most recently from 9de3605 to 027f570 Compare February 8, 2024 05:19

ChengyuZhu6 self-assigned this Feb 8, 2024

ChengyuZhu6 force-pushed the dependencies-for-guest-pull branch 5 times, most recently from 3cf3080 to 470d491 Compare February 8, 2024 09:11

fidencio reviewed Feb 9, 2024

View reviewed changes

fidencio force-pushed the dependencies-for-guest-pull branch from 470d491 to 7b1e8e8 Compare February 9, 2024 13:46

ChengyuZhu6 added 2 commits February 9, 2024 14:47

gha: Deploy nydus snapshotter by the daemonset

43b04fd

We can use daemonset to deploy nydus snapshotter, which will decrease one manual step both for Kata Containers and Confidential Containers CI. Fixes: kata-containers#8584 Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

gha: Cleanup nydus snapshotter by the daemonset

97fbf36

Cleanup nydus snapshotter by the daemonset. Signed-off-by: ChengyuZhu6 <chengyu.zhu@intel.com>

fidencio force-pushed the dependencies-for-guest-pull branch from 7b1e8e8 to 97fbf36 Compare February 9, 2024 13:47

fidencio approved these changes Feb 9, 2024

View reviewed changes

gkurz approved these changes Feb 9, 2024

View reviewed changes

stevenhorsman approved these changes Feb 9, 2024

View reviewed changes

fidencio merged commit e78a951 into kata-containers:main Feb 9, 2024
283 of 295 checks passed

BbolroC mentioned this pull request Feb 14, 2024

Local build for nydus snapshotter missing #9094

Open

ChengyuZhu6 deleted the dependencies-for-guest-pull branch February 18, 2024 04:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gha: Setup nydus snapshotter for CoCo tests #8585

gha: Setup nydus snapshotter for CoCo tests #8585

ChengyuZhu6 commented Dec 6, 2023 •

edited

stevenhorsman left a comment

fidencio commented Dec 6, 2023

ChengyuZhu6 commented Dec 7, 2023

ChengyuZhu6 commented Dec 7, 2023

ChengyuZhu6 commented Dec 7, 2023

liudalibj left a comment

stevenhorsman commented Dec 7, 2023

ChengyuZhu6 commented Feb 7, 2024

ChengyuZhu6 commented Feb 8, 2024

ChengyuZhu6 commented Feb 8, 2024

fidencio left a comment

fidencio Feb 9, 2024

fidencio Feb 9, 2024

fidencio Feb 9, 2024

fidencio Feb 9, 2024

fidencio Feb 9, 2024

fidencio left a comment

fidencio commented Feb 9, 2024

gkurz left a comment

stevenhorsman left a comment

stevenhorsman Feb 9, 2024

	KATA_HYPERVISOR=${KATA_HYPERVISOR-qemu}
	KATA_HYPERVISOR=${KATA_HYPERVISOR:-qemu}

gha: Setup nydus snapshotter for CoCo tests #8585

gha: Setup nydus snapshotter for CoCo tests #8585

Conversation

ChengyuZhu6 commented Dec 6, 2023 • edited

stevenhorsman left a comment

Choose a reason for hiding this comment

fidencio commented Dec 6, 2023

ChengyuZhu6 commented Dec 7, 2023

ChengyuZhu6 commented Dec 7, 2023

ChengyuZhu6 commented Dec 7, 2023

liudalibj left a comment

Choose a reason for hiding this comment

stevenhorsman commented Dec 7, 2023

ChengyuZhu6 commented Feb 7, 2024

ChengyuZhu6 commented Feb 8, 2024

ChengyuZhu6 commented Feb 8, 2024

fidencio left a comment

Choose a reason for hiding this comment

fidencio Feb 9, 2024

Choose a reason for hiding this comment

fidencio Feb 9, 2024

Choose a reason for hiding this comment

fidencio Feb 9, 2024

Choose a reason for hiding this comment

fidencio Feb 9, 2024

Choose a reason for hiding this comment

fidencio Feb 9, 2024

Choose a reason for hiding this comment

fidencio left a comment

Choose a reason for hiding this comment

fidencio commented Feb 9, 2024

gkurz left a comment

Choose a reason for hiding this comment

stevenhorsman left a comment

Choose a reason for hiding this comment

stevenhorsman Feb 9, 2024

Choose a reason for hiding this comment

ChengyuZhu6 commented Dec 6, 2023 •

edited