chore!: remove logging from init package #2625

lucasrod16 · 2024-06-13T23:22:31Z

Description

Removes PLG logging stack from the init package

Related Issue

Fixes #2582
Relates to #2562

Checklist before merging

Test, docs, adr added or updated as needed
Contributor Guide Steps followed

netlify · 2024-06-13T23:22:49Z

✅ Deploy Preview for zarf-docs ready!

Name	Link
🔨 Latest commit	`9846790`
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/667b2c20cc12be0008ca2a7b
😎 Deploy Preview	https://deploy-preview-2625--zarf-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

src/internal/packager/helm/zarf.go

Noxsios

Good stuff @lucasrod16

Only cursory stuff left imo:

Make sure to label this PR as breaking w/ ! || BREAKING
Make sure to callout in all the slacks this change is coming and migration strategies for people currently using

In the announcement it would be cool to show a rough estimate how much time in CI this saves (ltm ~ 2-3 mins rn), how much smaller the init package is and the compounding effects of that (like how this decreases every UDS-Core bundle by X bytes)

src/internal/packager/helm/zarf.go

schristoff-du

I reviewed this earlier and never submitted 🙃

src/types/k8s.go

## Description This PR splits out a refactor that was done in #2625 #2625 (comment) Here is some context for why I refactored `UpdateZarfAgentValues`. In our e2e test suite, `TestConnectAndCreds` runs `zarf tools update-creds --confirm`. `update-creds` calls `UpdateZarfAgentValues` which used to delete all agent pods manually based on label selector and immediately return without waiting for the pods to be ready. `TestMetrics` is ran immediately after `TestConnectAndCreds`, and it attempts to establish a tunnel connection to the Zarf agent to test the /metrics endpoint. This test was failing because there were no running agent pods yet after deleting them. This is not a problem in our tests right now because we run assertions against the logging stack in the cluster in `TestConnectAndCreds`, which buys us enough time for the agent pods to come back up and running. I refactored `UpdateZarfAgentValues` to perform a rolling update of the agent deployment by modifying the Pod template rather than manually deleting pods, and added a wait to ensure the agent deployment is ready before returning. This approach ensures a controlled and graceful update process managed by Kubernetes and simplifies the code. Co-authored-by: Austin Abro <[email protected]>

codecov · 2024-06-21T15:44:05Z

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 18.34%. Comparing base (7cd2e16) to head (9846790).

Files	Patch %	Lines
src/internal/packager/template/template.go	0.00%	1 Missing ⚠️
src/pkg/message/credentials.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2625      +/-   ##
==========================================
- Coverage   18.34%   18.34%   -0.01%     
==========================================
  Files         168      168              
  Lines       12082    12065      -17     
==========================================
- Hits         2217     2213       -4     
+ Misses       9607     9595      -12     
+ Partials      258      257       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lucasrod16 · 2024-06-24T17:06:08Z

Good stuff @lucasrod16

Only cursory stuff left imo:
* Make sure to label this PR as breaking w/ `!` || `BREAKING`

* Make sure to callout in all the slacks this change is coming and migration strategies for people currently using
In the announcement it would be cool to show a rough estimate how much time in CI this saves (ltm ~ 2-3 mins rn), how much smaller the init package is and the compounding effects of that (like how this decreases every UDS-Core bundle by X bytes)

Great suggestions. I will note the size differences here as well

amd64

v0.35.0 init package with PLG logging stack included - 515 megabytes
This PR's init package without PLG logging stack - 321 megabytes

Reduced in size by 194 megabytes, a 37.6% overall decrease.

arm64

v0.35.0 init package with PLG logging stack included - 476 megabytes
This PR's init package without PLG logging stack - 297 megabytes

Reduced in size by 179 megabytes, a 37.6% overall decrease.

phillebaba

It is difficult to with 100% certainty determine that we have not missed anything. I cant see anything directly wrong either. Removing unused components is great in decreasing the complexity.

## Description This PR splits out a refactor that was done in #2625 #2625 (comment) Here is some context for why I refactored `UpdateZarfAgentValues`. In our e2e test suite, `TestConnectAndCreds` runs `zarf tools update-creds --confirm`. `update-creds` calls `UpdateZarfAgentValues` which used to delete all agent pods manually based on label selector and immediately return without waiting for the pods to be ready. `TestMetrics` is ran immediately after `TestConnectAndCreds`, and it attempts to establish a tunnel connection to the Zarf agent to test the /metrics endpoint. This test was failing because there were no running agent pods yet after deleting them. This is not a problem in our tests right now because we run assertions against the logging stack in the cluster in `TestConnectAndCreds`, which buys us enough time for the agent pods to come back up and running. I refactored `UpdateZarfAgentValues` to perform a rolling update of the agent deployment by modifying the Pod template rather than manually deleting pods, and added a wait to ensure the agent deployment is ready before returning. This approach ensures a controlled and graceful update process managed by Kubernetes and simplifies the code. Co-authored-by: Austin Abro <[email protected]> Signed-off-by: Austin Abro <[email protected]>

lucasrod16 force-pushed the remove-logging-from-init-pkg branch 4 times, most recently from bdbdae4 to e52f257 Compare June 14, 2024 01:03

lucasrod16 mentioned this pull request Jun 14, 2024

test: reduce duration of E2E tests #2562

Closed

lucasrod16 force-pushed the remove-logging-from-init-pkg branch 2 times, most recently from e65aa2e to 7979709 Compare June 14, 2024 18:57

lucasrod16 commented Jun 14, 2024

View reviewed changes

src/internal/packager/helm/zarf.go Outdated Show resolved Hide resolved

lucasrod16 force-pushed the remove-logging-from-init-pkg branch 4 times, most recently from 28035d0 to dcde625 Compare June 18, 2024 14:07

lucasrod16 marked this pull request as ready for review June 18, 2024 16:40

lucasrod16 requested review from dgershman and a team as code owners June 18, 2024 16:40

lucasrod16 added the ready-for-review label Jun 18, 2024

Noxsios reviewed Jun 18, 2024

View reviewed changes

src/internal/packager/helm/zarf.go Outdated Show resolved Hide resolved

lucasrod16 mentioned this pull request Jun 20, 2024

refactor: change UpdateZarfAgentValues to rolling restart #2644

Merged

lucasrod16 changed the title ~~chore: remove logging from init package~~ chore!: remove logging from init package Jun 20, 2024

lucasrod16 added the breaking-change 💔 label Jun 20, 2024

schristoff-du reviewed Jun 21, 2024

View reviewed changes

src/types/k8s.go Show resolved Hide resolved

lucasrod16 force-pushed the remove-logging-from-init-pkg branch from dcde625 to 522c20b Compare June 21, 2024 15:38

lucasrod16 force-pushed the remove-logging-from-init-pkg branch from 522c20b to fc84edc Compare June 21, 2024 16:15

AustinAbro321 previously approved these changes Jun 21, 2024

View reviewed changes

lucasrod16 dismissed AustinAbro321’s stale review via e3d35cd June 24, 2024 16:10

lucasrod16 force-pushed the remove-logging-from-init-pkg branch from fc84edc to e3d35cd Compare June 24, 2024 16:10

phillebaba previously approved these changes Jun 25, 2024

View reviewed changes

chore!: remove logging from init package

9846790

lucasrod16 dismissed phillebaba’s stale review via 9846790 June 25, 2024 20:44

lucasrod16 force-pushed the remove-logging-from-init-pkg branch from 1ccec38 to 9846790 Compare June 25, 2024 20:44

schristoff-du approved these changes Jun 25, 2024

View reviewed changes

AustinAbro321 approved these changes Jun 26, 2024

View reviewed changes

lucasrod16 merged commit a095700 into main Jun 26, 2024
33 checks passed

lucasrod16 deleted the remove-logging-from-init-pkg branch June 26, 2024 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore!: remove logging from init package #2625

chore!: remove logging from init package #2625

lucasrod16 commented Jun 13, 2024 •

edited

Loading

netlify bot commented Jun 13, 2024 •

edited

Loading

Noxsios left a comment •

edited

Loading

schristoff-du left a comment

codecov bot commented Jun 21, 2024 •

edited

Loading

lucasrod16 commented Jun 24, 2024 •

edited

Loading

phillebaba left a comment

chore!: remove logging from init package #2625

chore!: remove logging from init package #2625

Conversation

lucasrod16 commented Jun 13, 2024 • edited Loading

Description

Related Issue

Checklist before merging

netlify bot commented Jun 13, 2024 • edited Loading

✅ Deploy Preview for zarf-docs ready!

Noxsios left a comment • edited Loading

Choose a reason for hiding this comment

schristoff-du left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2024 • edited Loading

Codecov Report

lucasrod16 commented Jun 24, 2024 • edited Loading

amd64

arm64

phillebaba left a comment

Choose a reason for hiding this comment

lucasrod16 commented Jun 13, 2024 •

edited

Loading

netlify bot commented Jun 13, 2024 •

edited

Loading

Noxsios left a comment •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading

lucasrod16 commented Jun 24, 2024 •

edited

Loading