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

Enforce and default Kafka configuration #896

Merged
merged 2 commits into from
Jan 3, 2024

Conversation

nasark
Copy link
Member

@nasark nasark commented Oct 6, 2022

  • Enforce Kafka by making the orchestrator wait for Kafka service to be ready (requires Kafka network policy)
  • default deployMessagingService to true from CR as Kafka will be the default configuration -> this will retain the option to disable Kafka in case things go wrong

Depends on:

Needed for:

Ref:

@miq-bot add_label enhancement
@miq-bot assign @Fryguy
@miq-bot add_reviewer @agrare

@nasark nasark requested a review from bdunne as a code owner October 6, 2022 14:17
@miq-bot miq-bot requested a review from agrare October 6, 2022 14:18
@Fryguy
Copy link
Member

Fryguy commented Oct 13, 2022

@bdunne Please review.

@agrare
Copy link
Member

agrare commented Oct 21, 2022

@nasark so this removes the option to deploy kafka, does this actually do the deployment?

@nasark
Copy link
Member Author

nasark commented Oct 21, 2022

@nasark so this removes the option to deploy kafka, does this actually do the deployment?

@agrare Actually this removes the flag that would disable Kafka by default. Removing the flag altogether makes it so that Kafka is deployed alongside the other services by default.

@agrare
Copy link
Member

agrare commented Oct 21, 2022

Awesome thanks @nasark can you tell what it deploys is it ManageIQ/container-kafka ?

@nasark
Copy link
Member Author

nasark commented Oct 21, 2022

If removing the flag altogether is a concern for backwards compatibility, then we could just set the default value for deployMessagingService to true as well. Some clarification/elaboration regarding #896 (comment) would be appreciated @Fryguy

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2022

If removing the flag altogether is a concern for backwards compatibility, then we could just set the default value for deployMessagingService to true as well. Some clarification/elaboration regarding #896 (comment) would be appreciated @Fryguy

We may also need a CR migration, but it might not matter if we just don't honor it? cc @bdunne

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2022

Awesome thanks @nasark can you tell what it deploys is it ManageIQ/container-kafka ?

It would be whatever the value is in the CR - https://github.com/ManageIQ/manageiq-pods/blob/master/manageiq-operator/config/crd/bases/manageiq.org_manageiqs.yaml#L139-L148 seems to show bitnami/kafka and https://github.com/ManageIQ/manageiq-pods/blob/master/manageiq-operator/config/crd/bases/manageiq.org_manageiqs.yaml#L306-L316 shows bitnami/zookeeper.

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2022

@agrare That would explain why that image hasn't been updated in years 😆

@agrare agrare mentioned this pull request Nov 9, 2022
6 tasks
@nasark
Copy link
Member Author

nasark commented Feb 3, 2023

Returning to this, perhaps it is easier to just default deployMessagingService to true instead of removing it from the CR altogether. Let me know what you think and how we can proceed with this @Fryguy @bdunne

@nasark
Copy link
Member Author

nasark commented Feb 9, 2023

I've split some of this up as it was getting big, and moved into other PRs based on issues I have found in my testing:

@nasark nasark force-pushed the require_kafka branch 3 times, most recently from 3f647c0 to e04bb0e Compare April 19, 2023 19:58
@miq-bot
Copy link
Member

miq-bot commented Jul 31, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@nasark
Copy link
Member Author

nasark commented Jul 31, 2023

@miq-bot remove_label stale

@miq-bot miq-bot removed the stale label Jul 31, 2023
@nasark nasark mentioned this pull request Nov 2, 2023
11 tasks
@miq-bot miq-bot added the stale label Nov 6, 2023
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s)

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@nasark nasark force-pushed the require_kafka branch 2 times, most recently from f7abcf1 to c2ec854 Compare November 6, 2023 15:03
@nasark
Copy link
Member Author

nasark commented Nov 6, 2023

@miq-bot remove_label stale

@miq-bot miq-bot removed the stale label Nov 6, 2023
@nasark
Copy link
Member Author

nasark commented Dec 19, 2023

@miq-bot add_label quinteros/yes?

@nasark nasark force-pushed the require_kafka branch 2 times, most recently from 71762c0 to a153fd7 Compare January 2, 2024 21:42
@miq-bot
Copy link
Member

miq-bot commented Jan 3, 2024

Checked commits nasark/manageiq-pods@2a5faca~...4d4fdbf with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@bdunne bdunne merged commit 09da4c7 into ManageIQ:master Jan 3, 2024
2 checks passed
@Fryguy
Copy link
Member

Fryguy commented Jan 5, 2024

Backported to quinteros in commit fe79478.

commit fe79478dc8a65610d2743801b4973825fffe5985
Author: Brandon Dunne <[email protected]>
Date:   Wed Jan 3 12:06:32 2024 -0500

    Merge pull request #896 from nasark/require_kafka
    
    Enforce and default Kafka configuration
    
    (cherry picked from commit 09da4c7f3083fad1b56f345d7e6e8c0063a37f58)

Fryguy pushed a commit that referenced this pull request Jan 5, 2024
Enforce and default Kafka configuration

(cherry picked from commit 09da4c7)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants