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

WIP Ensure ActiveGate is Enabled for Extensions #3361

Closed
wants to merge 2 commits into from

Conversation

wepudt
Copy link
Collaborator

@wepudt wepudt commented Jun 26, 2024

Description

see K8S-10118

ActiveGate is also created when Extensions are enabled in the DynaKube. No other AG capabilities are needed.

Dynakube v1beta2 is manually converted to v1beta3 in all cases when dynakube.NeedsActiveGate() or NeedsActiveGateService() is called.

How can this be tested?

  1. Unittests.
  2. Apply dynakubes below. ActiveGate pod should be always created.
    Check enabled capabilities on the AG pod using:
kubectl -n dynatrace get -o yaml pod/dynakube-activegate-0 -o jsonpath="{.spec.containers[0].env[0]}"

a)
dynakube:

spec:
  activeGate:
    capabilities:
      - routing
    replicas: 1
  extensions:
    prometheus:
      enabled: true
  oneAgent:
    cloudNativeFullStack: {}

capabilities:

{"name":"DT_CAPABILITIES","value":"MSGrouter,extension_controller"}

b)
dynakube:

spec:
  activeGate:
    replicas: 1
  extensions:
    prometheus:
      enabled: true
  oneAgent:
    cloudNativeFullStack: {}

capabilities:

{"name":"DT_CAPABILITIES","value":"extension_controller"}

c)
dynakube:

spec:
  extensions:
    prometheus:
      enabled: true
  oneAgent:
    cloudNativeFullStack: {}

capabilities:

{"name":"DT_CAPABILITIES","value":"extension_controller"}

@wepudt wepudt changed the title enable AG when extensions are enabled in DK Ensure ActiveGate is Enabled for Extensions Jun 26, 2024
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 74.31694% with 94 lines in your changes missing coverage. Please review.

Project coverage is 57.13%. Comparing base (79211a9) to head (0b753a4).

Files Patch % Lines
...mocks/pkg/controllers/dynakube/istio/reconciler.go 13.33% 12 Missing and 1 partial ⚠️
...cks/pkg/controllers/dynakube/version/reconciler.go 33.33% 12 Missing ⚠️
cmd/troubleshoot/image.go 16.66% 9 Missing and 1 partial ⚠️
.../dynakube/deploymentmetadata/deploymentmetadata.go 37.50% 10 Missing ⚠️
...ntrollers/dynakube/oneagent/oneagent_reconciler.go 37.50% 6 Missing and 4 partials ⚠️
pkg/controllers/dynakube/controller.go 73.07% 5 Missing and 2 partials ⚠️
pkg/api/v1beta3/dynakube/properties.go 0.00% 6 Missing ⚠️
pkg/controllers/dynakube/injection/reconciler.go 62.50% 3 Missing and 3 partials ⚠️
...s/dynakube/processmoduleconfigsecret/reconciler.go 25.00% 6 Missing ⚠️
pkg/controllers/dynakube/phase.go 84.00% 2 Missing and 2 partials ⚠️
... and 5 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3361      +/-   ##
==========================================
- Coverage   57.23%   57.13%   -0.11%     
==========================================
  Files         344      344              
  Lines       19781    19849      +68     
==========================================
+ Hits        11322    11340      +18     
- Misses       7221     7256      +35     
- Partials     1238     1253      +15     
Flag Coverage Δ
unittests 57.13% <74.31%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@wepudt wepudt force-pushed the feature/enable_extension_controller_activegate branch from d3e8730 to 836a49b Compare July 5, 2024 07:55
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch 16 times, most recently from d6a5873 to 7702ab9 Compare July 8, 2024 17:23
aorcholski
aorcholski previously approved these changes Jul 8, 2024
@aorcholski aorcholski self-requested a review July 8, 2024 17:24
@aorcholski aorcholski requested a review from 0sewa0 July 9, 2024 09:22
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch from 7702ab9 to 74de5f2 Compare July 9, 2024 11:35
@aorcholski aorcholski marked this pull request as ready for review July 9, 2024 12:01
@aorcholski aorcholski requested a review from a team as a code owner July 9, 2024 12:01
@aorcholski aorcholski requested a review from gkrenn July 9, 2024 12:01
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch 2 times, most recently from a7b24ca to 0c66a37 Compare July 9, 2024 13:33
Comment on lines +7 to +8
"github.com/Dynatrace/dynatrace-operator/pkg/api/v1beta2/dynakube"
dynakubev1beta3 "github.com/Dynatrace/dynatrace-operator/pkg/api/v1beta3/dynakube"
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, if you use 2 versions, use 2 aliases

Copy link
Contributor

@0sewa0 0sewa0 Jul 9, 2024

Choose a reason for hiding this comment

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

I read through more of the code, and it always comes back up.
And I started to ask my self: "What is the benefit of handling 2 versions in a package?"

It is just confusing, and will only cause more work, because when we move away from the older version, all of these packages need to be changed.
There is no information loss when converting between the versions, so what the old-version had the new-version also has. So doing this "use double the version, and sometimes pass in 2 dynakubes" are just asking for mistakes caused by confusion.

IMO The biggest flaw with this whole approach (converting at "random" places) is that we like to pass the DynaKube via reference, and we expect the Status part to be mutated as we pass it around, but the moment you introduce conversions in places, and not handle the mutation carefully, some data may be lost

How I would do it is:

  • Only 1 version is allowed in a package (conversion and main dk-controller is exempt, maybe troubleshoot and istio aswell)
  • If a package uses the new-version in any way, the input for that package should be the new-version
    • The caller of the package will do the convertTo and From, so it will own the reference that is passed it, so no changes to the Status should be lost.

Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree with you, I think we should focus on unblocking this PR (because Werner is on vacation, many stories depends on this and it's getting stucked) and then start switching to beta3 everywhere. We will have to do it anyways with the oncoming stories so we can do it in small increments, or maybe do it all at once after this is merged?

I'm just trying to find the most painless way of moving forward with this.

@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch from 0c66a37 to c3c709b Compare July 10, 2024 10:39
@aorcholski aorcholski changed the title Ensure ActiveGate is Enabled for Extensions WIP Ensure ActiveGate is Enabled for Extensions Jul 10, 2024
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch from c3c709b to 8ed6f57 Compare July 10, 2024 16:56
@aorcholski aorcholski changed the title WIP Ensure ActiveGate is Enabled for Extensions Ensure ActiveGate is Enabled for Extensions Jul 10, 2024
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch 4 times, most recently from 8759b85 to 8d004dc Compare July 11, 2024 08:04
@albertogdd albertogdd requested a review from 0sewa0 July 11, 2024 08:27
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch 6 times, most recently from 6837f17 to e8af565 Compare July 11, 2024 10:12
@aorcholski aorcholski force-pushed the feature/enable_extension_controller_activegate branch from e8af565 to 0b753a4 Compare July 11, 2024 10:14
@aorcholski aorcholski changed the title Ensure ActiveGate is Enabled for Extensions WIP Ensure ActiveGate is Enabled for Extensions Jul 12, 2024
@aorcholski aorcholski closed this Jul 15, 2024
@aorcholski
Copy link
Contributor

new PR based on v1beta3 branch will be created

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

Successfully merging this pull request may close these issues.

None yet

6 participants