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.

Sign up for GitHub

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

Jump to bottom

misc: fix 404 for snapshotter dockerfile #312

Merged
merged 1 commit into from
Jan 13, 2023
Merged

misc: fix 404 for snapshotter dockerfile #312

merged 1 commit into from
Jan 13, 2023

Conversation

sctb512
Copy link
Member

@sctb512 sctb512 commented Jan 12, 2023

The github workflow frequently causes errors to break tests:

Step 4/18 : RUN export NYDUS_VERSION=$(curl --silent "https://api.github.com/repos/dragonflyoss/image-service/releases/latest" | grep -Po '"tag_name": "\K.*?(?=")') &&     wget https://github.com/dragonflyoss/image-service/releases/download/$NYDUS_VERSION/nydus-static-$NYDUS_VERSION-linux-amd64.tgz &&     echo $NYDUS_VERSION > /.nydus_version &&     tar xzf nydus-static-$NYDUS_VERSION-linux-amd64.tgz &&     rm nydus-static-$NYDUS_VERSION-linux-amd64.tgz
 ---> Running in 39df2748e06d
Connecting to github.com (140.82.112.4:443)
wget: server returned error: HTTP/1.1 404 Not Found
The command '/bin/sh -c export NYDUS_VERSION=$(curl --silent "https://api.github.com/repos/dragonflyoss/image-service/releases/latest" | grep -Po '"tag_name": "\K.*?(?=")') &&     wget https://github.com/dragonflyoss/image-service/releases/download/$NYDUS_VERSION/nydus-static-$NYDUS_VERSION-linux-amd64.tgz &&     echo $NYDUS_VERSION > /.nydus_version &&     tar xzf nydus-static-$NYDUS_VERSION-linux-amd64.tgz &&     rm nydus-static-$NYDUS_VERSION-linux-amd64.tgz' returned a non-zero code: 1

I guess the reason for this situation is the $NYDUS_VERSION is an invalid value.
This PR tries to verify it by adding an echo command.

Signed-off-by: Bin Tang tangbin.bin@bytedance.com

@sctb512
Copy link
Member Author

sctb512 commented Jan 12, 2023

Yes, that is right. The version in a null value.

version:
Connecting to github.com (140.82.114.4:443)
wget: server returned error: HTTP/1.1 404 Not Found

@sctb512 sctb512 force-pushed the fix-github-workflow branch 12 times, most recently from 52b68e1 to e939cde Compare January 12, 2023 02:52
fengshunli
fengshunli reviewed Jan 12, 2023
View reviewed changes
Copy link
Contributor

@fengshunli fengshunli left a comment

Choose a reason for hiding this comment

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

This needs to support arm64

@sctb512
Copy link
Member Author

sctb512 commented Jan 12, 2023

This needs to support arm64

Do you mean that we should add a variable to control the architecture?

nydus-static-$NYDUS_VERSION-linux-$NYDUS_ARCHITECTURE.tgz

@fengshunli
Copy link
Contributor

This needs to support arm64

Do you mean that we should add a variable to control the architecture?

nydus-static-$NYDUS_VERSION-linux-$NYDUS_ARCHITECTURE.tgz

Yes, add add ARG TARGETARCH environment variable

@sctb512
Copy link
Member Author

sctb512 commented Jan 12, 2023

Done. PTAL @fengshunli

@sctb512 sctb512 force-pushed the fix-github-workflow branch 2 times, most recently from 519d68f to 9ed4474 Compare January 12, 2023 06:06
@fengshunli
Copy link
Contributor

fengshunli commented Jan 12, 2023 •
edited
Loading

refer to ARG TARGETARCH way to achieve

@sctb512 sctb512 force-pushed the fix-github-workflow branch 3 times, most recently from b893591 to 0f7c3de Compare January 12, 2023 06:33
@sctb512
Copy link
Member Author

sctb512 commented Jan 12, 2023

refer to ARG TARGETARCH way to achieve

Fixed.

@fengshunli
Copy link
Contributor

LGTM

@liubin
Copy link
Contributor

liubin commented Jan 12, 2023 •
edited
Loading

It may be better to add a sleep 1 in each loop, because GitHub may limit the request frequency, retry immediately will continue to get a 403 error.

changweige
changweige reviewed Jan 12, 2023
View reviewed changes
Copy link
Member

@changweige changweige left a comment

Choose a reason for hiding this comment

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

I am wondering why NYDUS_VERSION is empty.

@changweige
Copy link
Member

changweige commented Jan 12, 2023 •
edited
Loading

It may be better to add a sleep 1 in one loop, because GitHub may limit the request frequency, retry immediately will continue to get a 403 error.

Agree. It might be because nydus-snapshotter is under Containerd org where a lot of tests are running at the same time

changweige
changweige reviewed Jan 13, 2023
View reviewed changes
misc/snapshotter/Dockerfile Outdated
wget https://github.com/dragonflyoss/image-service/releases/download/$NYDUS_VERSION/nydus-static-$NYDUS_VERSION-linux-amd64.tgz && \
RUN unset NYDUS_VERSION && \
while [ -z $NYDUS_VERSION ]; do \
export NYDUS_VERSION=$(curl --silent "https://api.github.com/repos/dragonflyoss/image-service/releases/latest" | grep -Po '"tag_name": "\K.*?(?=")'); sleep 1; \
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 better to limit the total retry times.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

@sctb512 sctb512 Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It still failed when i retry 1200 times. Maybe we could assume it will work eventually?

Copy link
Member Author

Choose a reason for hiding this comment

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

It works now. 10 minutes are enough.

changweige
changweige reviewed Jan 13, 2023
View reviewed changes
misc/snapshotter/Dockerfile Outdated
export NYDUS_VERSION=$(curl --silent "https://api.github.com/repos/dragonflyoss/image-service/releases/latest" | grep -Po '"tag_name": "\K.*?(?=")'); sleep 1; \
done && \
echo NYDUS_VERSION: $NYDUS_VERSION && \
if [ -z $NYDUS_VERSION ]; then echo Invalid NYDUS_VERSION && exit -1; fi && \
Copy link
Member

Choose a reason for hiding this comment

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

This condition judgment conflicts with line 8, which means the loop should not end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Now, we limit the total retry times, so we still need this.

changweige
changweige reviewed Jan 13, 2023
View reviewed changes
misc/snapshotter/Dockerfile Outdated
echo NYDUS_VERSION: $NYDUS_VERSION && \
if [ -z $NYDUS_VERSION ]; then echo Invalid NYDUS_VERSION && exit -1; fi && \
echo TARGETARCH: $TARGETARCH && \
if [ "$TARGETARCH" != "amd64" -a "$TARGETARCH" != "arm64" -a "$TARGETARCH" != "ppc64le" -a "$TARGETARCH" != "riscv64" ]; then echo Unsupported TARGETARCH for nydus && exit -1; fi && \
Copy link
Member

Choose a reason for hiding this comment

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

I still incline to simplify the test by assuming that only amd64 is the base nydus artifacts platform since the test target platform is decided and can't changed

@sctb512 sctb512 force-pushed the fix-github-workflow branch 3 times, most recently from 3506ccc to e8c152d Compare January 13, 2023 02:28
@sctb512
Copy link
Member Author

sctb512 commented Jan 13, 2023

Dockerfile with ARG TARGETARCH is more adaptable in multiple architectures. However, this Dockerfile is only for the E2E test. It is better to keep things simple. 😊
Maybe we could open a new PR to solve this requirement?

@sctb512 sctb512 force-pushed the fix-github-workflow branch 3 times, most recently from 09b8e02 to c3d0977 Compare January 13, 2023 02:48
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023 •
edited
Loading

Codecov Report

Base: 31.55% // Head: 31.55% // No change to project coverage 👍

Coverage data is based on head (af5d85d) compared to base (744cf77).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage   31.55%   31.55%           
=======================================
  Files          34       34           
  Lines        3578     3578           
=======================================
  Hits         1129     1129           
  Misses       2333     2333           
  Partials      116      116

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sctb512 sctb512 force-pushed the fix-github-workflow branch 13 times, most recently from d318cd3 to 04e9ca2 Compare January 13, 2023 03:42
@sctb512
misc: fix 404 for snapshotter dockerfile
af5d85d 
Signed-off-by: Bin Tang <tangbin.bin@bytedance.com>
@changweige
Copy link
Member

Dockerfile with ARG TARGETARCH is more adaptable in multiple architectures. However, this Dockerfile is only for the E2E test. It is better to keep things simple. 😊 Maybe we could open a new PR to solve this requirement?

Agree.

@changweige
Copy link
Member

Thanks for investigating this 👍

@changweige changweige merged commit 2e095fe into containerd:main Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengshunli fengshunli fengshunli left review comments

@changweige changweige changweige approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@sctb512 @fengshunli @liubin @changweige @codecov-commenter