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

feat(operator): update pod definition and nifi reconciliation for istio compatibility #476

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

nfoucha
Copy link
Contributor

@nfoucha nfoucha commented Nov 7, 2024

Q A
Bug fix? no
New feature? yes
API breaks? no
Deprecations? no
Related tickets partially #172
License Apache 2.0

What's in this PR?

This pushes the zookeeper connectivity check into the pod command instead of the init container. Because the nifi image doesn't have netcat, the implementation was adjusted to use curl with a telnet address and a bad option so it immediately terminates the connection. This results in a specific exit code 48 when the connectivity check passes. However, the overall logic is unchanged.

The reconciliation checks were also adjusted to remove prometheus annotations and the liveness probe definition from consideration when checking for the difference between the currently running and the "desired" pod state when the pod has been affected by istio injection. Additional containers present on the current pod spec are carried over to the desired pod spec to ensure injected containers don't trigger reconciliation.

Additional context

There are a couple of caveats with this approach to Istio compatibility that are important to call out. I believe these can be addressed via an architecture change to utilize a StatefulSet over reconciling the individual pods directly, but that was out of scope for what I'm trying to accomplish with this PR.

  • If an incoming spec change only targets the liveness probe definition, the operator will ignore the change and not reconcile the pods. This can be worked around by adding/removing annotations/labels.
  • If an incoming spec change removes custom init containers and/or sidecars, the operator will ignore the change and not reconcile the pods. This can be worked around by adding/removing annotations/labels.

Checklist

  • Implementation tested
  • Error handling code meets the guideline
  • Logging code meets the guideline
  • User guide and development docs updated (if needed)
  • Append changelog with changes

To Do

@nfoucha
Copy link
Contributor Author

nfoucha commented Nov 7, 2024

Worth mentioning that I tested this with zookeeper (1.28.0, 2.0.0) as well as the new kubernetes state management (2.0.0)

@nfoucha nfoucha changed the title feat(operator): update pod definition and nifi reconciliation for isto compatibility feat(operator): update pod definition and nifi reconciliation for istio compatibility Nov 7, 2024
Copy link
Contributor

@juldrixx juldrixx left a comment

Choose a reason for hiding this comment

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

Can you update the CHANGELOG.md file to reflect your changes?

@juldrixx
Copy link
Contributor

Can you explain me why these changes are necessary for Istio?
I'm pretty sure we were making it work when we wrote this https://konpyutaika.github.io/nifikop/docs/3_manage_nifi/1_manage_clusters/1_deploy_cluster/3_expose_cluster/2_istio_service_mesh but we moved from it so not sure if things change since then.

@juldrixx
Copy link
Contributor

Can you update the CHANGELOG.md file to reflect your changes?

And can also rebase your PR and check if everything is still fine (we never know 😀).

@nfoucha
Copy link
Contributor Author

nfoucha commented Nov 15, 2024

When using an init container that requires network connectivity, and in an environment with restrictive NetworkPolicy objects, it will fail continuously because the istio-init container has not completed setting up the iptables for the pod. By moving the Zookeeper networking check to the main container instead it eliminates the race condition and waits until the istio networking setup is complete before attempting a connection.

…io compatibility

this pushes the zk connectivity check into the pod command instead of the init container and removes istio overrides from reconciliation checks

relates to konpyutaika#172
@nfoucha nfoucha force-pushed the fix/istio-friendly-zk-check branch from ba59f14 to 66db052 Compare November 15, 2024 15:14
@nfoucha
Copy link
Contributor Author

nfoucha commented Nov 15, 2024

The other changes relate to the mutations performed on the pod by istio injection. This causes the reconciler to incorrectly flag the pods as out of sync and constantly recreates them as a result. By adjusting the "desired" pod spec to include the mutated parts of the pod, this reconciliation infinite loop is fixed.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Juldrixx <[email protected]>
@juldrixx
Copy link
Contributor

Thanks for the contribution. Will merge after the pipeline ends.

@juldrixx juldrixx merged commit a53ef52 into konpyutaika:master Nov 15, 2024
5 checks passed
@nfoucha nfoucha deleted the fix/istio-friendly-zk-check branch November 18, 2024 15:43
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.

2 participants