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

bubble up the error message from go-getter #18444

Merged
merged 4 commits into from
Sep 13, 2023
Merged

Conversation

shantanugadgil
Copy link
Contributor

@shantanugadgil shantanugadgil commented Sep 11, 2023

possible fix for #18442

EDIT:

Tested the locally built Nomad agent binary using nomad agent -dev with:

mkdir large_tar
cd large_tar/
mkdir data
for i in {1..5000} ; do echo "" > data/$i.txt; done
tar czvf data.tar.gz data/
python3 -m http.server 9000
job "tar_fail" {

  group "cache" {
    network {
      port "db" {
        to = 6379
      }
    }

    task "redis" {
      artifact {
        source      = "http://127.0.0.1:9000/data.tar.gz"
        destination = "local/some-directory"
      }

      driver = "docker"

      config {
        image          = "redis:7"
        ports          = ["db"]
        auth_soft_fail = true
      }

      identity {
        env  = true
        file = true
      }

      resources {
        cpu    = 500
        memory = 256
      }
    }
  }
}

@shantanugadgil
Copy link
Contributor Author

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

@shantanugadgil this doesn't actually get what you've described in #18442 does it? This added the inner error to the artifact block but that's only going to show up in Task Events, not the alloc logs. But I think that's actually what we want.

helper/subproc/subproc.go Outdated Show resolved Hide resolved
@shantanugadgil
Copy link
Contributor Author

@shantanugadgil this doesn't actually get what you've described in #18442 does it? This added the inner error to the artifact block but that's only going to show up in Task Events, not the alloc logs. But I think that's actually what we want.

I meant to write alloc status in the original bug report. Bubbling the error into alloc status is good enough.

Signed-off-by: Shantanu Gadgil <[email protected]>
helper/subproc/subproc.go Outdated Show resolved Hide resolved
Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

Hi @shantanugadgil! I've left a couple more comments. Also, can you add a changelog via make cl?

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM

I've smoke tested this with a broken artifact download and get task events like the following:

Recent Events:
Time                       Type                      Description
2023-09-13T09:35:00-04:00  Not Restarting            Policy allows no restarts
2023-09-13T09:35:00-04:00  Failed Artifact Download  failed to download artifact "s3::http://192.168.1.221:9000/test/release": getter subprocess failed: exit status 1: failed to download artifact: error downloading 'http://192.168.1.221:9000/test/release?aws_access_key_id=key&aws_access_key_secret=password1': RequestError: send request failed
caused by: Get "http://192.168.1.221:9000/test?prefix=release": dial tcp 192.168.1.221:9000: connect: connection refused
2023-09-13T09:35:00-04:00  Downloading Artifacts     Client is downloading artifacts
2023-09-13T09:34:59-04:00  Task Setup                Building Task Directory
2023-09-13T09:34:59-04:00  Received                  Task received by client

@tgross tgross added this to the 1.6.x milestone Sep 13, 2023
@tgross tgross merged commit 12580c3 into hashicorp:main Sep 13, 2023
@tgross tgross added the backport/1.6.x backport to 1.6.x release line label Sep 13, 2023
@tgross
Copy link
Member

tgross commented Sep 13, 2023

I've got a backport PR baking in #18479 so once that's green I'll merge it and that'll get shipped in the upcoming Nomad 1.6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

2 participants