Skip to content
This repository was archived by the owner on Dec 21, 2021. It is now read-only.

Added improved error handling around downloading and installing packages#326

Merged
siegfriedweber merged 9 commits intomainfrom
gzip_encoding
Oct 19, 2021
Merged

Added improved error handling around downloading and installing packages#326
siegfriedweber merged 9 commits intomainfrom
gzip_encoding

Conversation

@soenkeliebau
Copy link
Copy Markdown
Member

@soenkeliebau soenkeliebau commented Oct 10, 2021

The agent currently does not delete the target directory for package installations, if the installation fails. This PR adds handling to remove those directories on errors when extracting.

The agent now also specifically requests 'application/gzip' as content type.
We encountered issues with a webserver which provided our gzipped packages as 'application/tar' and moved the gzip to 'content_encoding' which caused reqwest to transparrently unpack the zip and the subsequent attempt by the agent to unzip this file to fail.

fixes #324
fixes #325

Description

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)

…ges.

Agent now specifically requests 'application/gzip' as content type in the download request and fails the download if it receives anything else.
@soenkeliebau soenkeliebau requested a review from a team October 11, 2021 07:24
maltesander
maltesander previously approved these changes Oct 11, 2021
Copy link
Copy Markdown
Member

@maltesander maltesander left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment thread src/provider/repository/stackablerepository.rs Outdated
@soenkeliebau soenkeliebau requested a review from nightkr October 14, 2021 06:47
nightkr
nightkr previously approved these changes Oct 18, 2021
@siegfriedweber
Copy link
Copy Markdown
Member

siegfriedweber commented Oct 18, 2021

The clean up after a failed installation cannot be tested with an integration test.

The check of the content type could be automatically tested as follows:

  1. The test case creates an ad-hoc repository which returns an invalid content type.
  2. The test case waits until the reason in the pod status is "DownloadingBackoff".
  3. The test case deletes the ad-hoc repository and creates a new one which returns the expected content type.
  4. The test case waits until the pod was started successfully.

The effort implementing this test case is large. Its meaningfulness is limited because the status "DownloadingBackoff" could result from another issue. It is in general a problem to test intermediate results in the Stackable Agent. So I would refrain from implementing it.

I adapted the integration tests to return the expected content type (see stackabletech/agent-integration-tests#89).

@soenkeliebau, @maltesander, @teozkr Are you okay if we merge this request without an integration test case?

Copy link
Copy Markdown
Member

@siegfriedweber siegfriedweber left a comment

Choose a reason for hiding this comment

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

LGTM (after my changes 😁)

One downside of this approach is that the Agent tries to recover from a non-recoverable error.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent has trouble handling some formats of .tar.gz files The agent doesn't fully check success of package installations on repeated attempts

4 participants