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

Release: Update nydus snapshotter version #364

Merged

Conversation

portersrc
Copy link
Member

@portersrc portersrc commented Apr 24, 2024

This is updated to match the nydus snapshotter version (v0.13.11) from the v3.4.0 kata release.

This is needed for v0.9.0 release process.

This is updated to match the nydus snapshotter version (v0.13.11)
from the v3.4.0 kata release

Signed-off-by: Chris Porter <[email protected]>
rm nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-${ARCH}.tgz && \
curl -fOL --progress-bar ${NYDUS_SNAPSHOTTER_REPO}/releases/download/${NYDUS_SNAPSHOTTER_VERSION}/nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-linux-${ARCH}.tar.gz && \
tar xvzpf nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-linux-${ARCH}.tar.gz -C / && \
rm nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-linux-${ARCH}.tar.gz && \
mv /nydus-snapshotter/* ${NODE_DESTINATION}/bin/ && \
Copy link
Contributor

Choose a reason for hiding this comment

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

The v0.13.11 doesn't use the nydus-snapshotter prefix inside the tarball so you need to remove the lines 75,76 (mv and rm /nydus-snapshotter)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good catch. I think I made a slightly different but appropriate change and see enclave-cc test passing now

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

thanks @portersrc, this changes the repo to the official one, which is in sync with kata-containers. The only little issue is that that version doesn't use the prefix inside the tarball but is directly uncompressed as it should. Once you remove the 2 extra lines it should be good to go.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM

mkdir /nydus-snapshotter && \
tar xvzpf nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-linux-${ARCH}.tar.gz -C /nydus-snapshotter && \
rm nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-linux-${ARCH}.tar.gz && \
mv /nydus-snapshotter/bin/* ${NODE_DESTINATION}/bin/ && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Well this ensures only bin/* files will be deployed (no matter what nydus packs) but do you think it's necessary to create, move and remove the files? I think simple deployment directly to / would be more elegant (with some pros - like when nydus adds extra files, and cons - when nydus adds extra files we don't want to deploy)

Copy link
Contributor

Choose a reason for hiding this comment

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

Note it's not a strict requirement, just my preference. So feel free to say you really really want it this way and I will get along with this complicated construct...

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw it must be extracted to tar xvzpf nydus-snapshotter-${NYDUS_SNAPSHOTTER_VERSION}-${ARCH}.tgz -C ${NODE_DESTINATION}/bin/ && \ then everything works well...

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 thanks @ldoktor. I incorporated this suggested now, and it seems to still be passing (just awaiting one last runner again).

A few tweaks to pull the correct nydus-snapshotter tarball
are needed: the remote repo, the tarball suffix, the "linux"
string within the tarball name, no need for amd64 vs. x86_64
checking, and handling of extracted tarball slightly tweaked

Signed-off-by: Chris Porter <[email protected]>
Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @portersrc !

@wainersm
Copy link
Member

Hi @portersrc ! Can I get this merged or are you waiting on some other PR?

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thanks, simple is better :-) (although squashing it into a single commit would be even better but let's not nit-pick and get this in)

@ldoktor ldoktor merged commit d72d2c0 into confidential-containers:main Apr 29, 2024
10 checks passed
@portersrc
Copy link
Member Author

Thanks, guys. Yes this was OK to merge. We can perform the rest of v0.9.0 steps now, I believe.

@portersrc portersrc deleted the pre-release-fixes-v0.9.0 branch May 24, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants