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

Data injection stability improvements #644

Merged
merged 23 commits into from
Jul 27, 2022
Merged

Conversation

jeff-mccoy
Copy link
Contributor

@jeff-mccoy jeff-mccoy commented Jul 25, 2022

This PR addresses some race conditions present with data injection since the addition of the Multi-distro support in #153. This also addresses some test flakes experienced for the data injection test.

  • BREAKING: Removes the ability to specify data injections into generic pods vs pod + container
  • Adds a compress flag for data injections to enable gzip compression on transfer
  • Adds file copy activity logging while running the data injection
  • Adds new Helm Chart config option NoWait to allow helm to apply manifests and keep going (needed to address data injection race condition)
  • Adds final data injection pod wait loop after data transfer
  • Changes from kubectl cp exec to kubectl exec
  • Adds a dynamic sync completion marker so that data injections leveraging an initContainer don't stall on their second run
  • Removes the arbitrary timeout conditions breaking large data syncs on slow networks

Related Issue

Issue #643

@YrrepNoj YrrepNoj self-requested a review July 26, 2022 21:57
@jeff-mccoy jeff-mccoy added bug 🐞 Something isn't working tests labels Jul 27, 2022
@jeff-mccoy
Copy link
Contributor Author

@YrrepNoj @Racer159 this should be good for review now. Would like to merge in to stop breaking other PRs with the data injection flake.

YrrepNoj
YrrepNoj previously approved these changes Jul 27, 2022
Copy link
Contributor

@YrrepNoj YrrepNoj left a comment

Choose a reason for hiding this comment

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

Manually tested with a private package that was injecting >1.5GB of data

docs/4-user-guide/3-zarf-schema.md Outdated Show resolved Hide resolved
src/internal/helm/chart.go Outdated Show resolved Hide resolved
src/internal/template/template.go Show resolved Hide resolved
src/types/component.go Outdated Show resolved Hide resolved
Copy link
Contributor

@YrrepNoj YrrepNoj left a comment

Choose a reason for hiding this comment

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

Because @jeff-mccoy doesn't believe that I approve of his changes 👀

@jeff-mccoy jeff-mccoy merged commit 4cc5413 into master Jul 27, 2022
@jeff-mccoy jeff-mccoy deleted the data-injection-stability branch July 27, 2022 18:18
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
BREAKING: Removes the ability to specify data injections into generic pods vs pod + container
Adds a compress flag for data injections to enable gzip compression on transfer
Adds file copy activity logging while running the data injection
Adds new Helm Chart config option NoWait to allow helm to apply manifests and keep going (needed to address data injection race condition)
Adds final data injection pod wait loop after data transfer
Changes from kubectl cp exec to kubectl exec
Adds a dynamic sync completion marker so that data injections leveraging an initContainer don't stall on their second run
Removes the arbitrary timeout conditions breaking large data syncs on slow networks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐞 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants