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

[ST] Add NodePools to all of the test-cases, create NodePoolsConverter, add possibility to specify NP roles for all STs #9668

Merged
merged 5 commits into from Feb 19, 2024

Conversation

im-konge
Copy link
Member

@im-konge im-konge commented Feb 9, 2024

Type of change

  • Enhancement

Description

This PRs adds creation on NodePools to every test case we have in our systemtest module.
And because we want to run the tests in multiple modes -> ZK without NodePools, ZK with NodePools, KRaft -> I added NodePoolsConverter, which converts NodePools, that are passed into it, based on the mode. As a result, the final list of NodePools is returned and it can be passed to the resourceManager.createResourceWithWait method.

The NodePoolsConverter does following:

  1. takes the list that is provided for conversion
  2. in case that we run in MIXED mode (that means that the NodePool should have both broker and controller roles), the controller NodePools are removed and broker NodePools are updated with controller role
  3. otherwise this change is skipped
  4. after that, it filters the list
    • in case of ZK without NodePools, it returns empty list (so nothing will be created in that case)
    • in case of ZK with NodePools, it removes NodePools containing controller role
    • in case of KRaft, does nothing and returns the list without a change

Note: the conversion to mixed mode is done only in case that the KRaft mode is enabled + !Environment.isSeparateRolesMode()

Together with this, I had to change multiple things -> added new fields to TestStorage, so it contains all NodePools (controller, broker, mixed), I had to create new methods for returning correct "component name" and LabelSelectors based on the mode. KafkaResources.kafkaPodName() is not used anymore, as the pods "numbers" can be different - we could "work around" it and set the IDs by annotation, but I think that it's better to get the Pod name using list pods by LabelSelector, than to assume that the Pod will be named that way.

convertKafkaResourceToKafkaNodePool was removed, because it is not needed anymore - that was a workaround, so we were able to run the tests with NodePools, but it doesn't make sense to have it there, as we should follow the way how "user would use it".

Checklist

  • Make sure all tests pass

@im-konge
Copy link
Member Author

im-konge commented Feb 9, 2024

/packit test --labels regression

@im-konge
Copy link
Member Author

im-konge commented Feb 9, 2024

/packit test --labels upgrade

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Pull request contains merge conflicts.

@im-konge im-konge force-pushed the node-pools-handler branch 2 times, most recently from d6aefb5 to 6670ea1 Compare February 12, 2024 22:40
@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the pass 👍 thanks just one nit...

@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@henryZrncik henryZrncik left a comment

Choose a reason for hiding this comment

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

Pretty much works for me only confirmation question mainly about: NodePoolsConverter.java class.

Judging from the way it is used, commets you provided and code i assume it works as following:
1.) It is called whenever we need this logic to decide what to do about specific node Pool, otherwise we stick to direct creation of resource without this method.

2.) We are introducing Environment.IsSepareRoleModel default to true playing role only if is Environment.isNodePoolEnabled on (effectively it does most of its work only if Kraft is enabled as well) ?

3.) The working of the public static method convertNodePoolsIfNeeded
could be summarized by following ?

If ! Environment.IsKafkaNodePoolEnabled return empty list i.e. no resource created.
otherwise follow the table for each original node pool role role.

(first cell in table tells which Role is KNP, axis x-> if kraft Is Enabled axis y if SeparateRolesMode are enabled) rest of cells are telling outcome of what happens with given node pool.

(Mixed Role) IsKraft ! isKraft
isSeparateRole Mixed (keep) - (remove as contain controller)
!isSeparateRole Mixed (keep) - (remove as contain controller)
(Controller Role) IsKraft ! isKraft
isSeparateRole controller (keep) - (remove as contain controller)
!isSeparateRole Mixed (change) - (remove as contain controller)
(Broker) IsKraft ! isKraft
isSeparateRole broker (keep) broker (keep)
!isSeparateRole Mixed (change) - (remove as contain controller) it seems to be added in code

?

Anyway really great Job, Thanks Lukas.

@im-konge
Copy link
Member Author

@henryZrncik yeah the tables you provided are correct. Also worth mentioning is, that in our test-cases, we are not currently using the NodePoolsConverter with mixed role. And yes, it removes the NodePools as you mentioned - even if you have !isSeparateRole and !isKRaft, which should be changed - so it will skip the mixed role conversion in that case.
But that's case which will not happen currently anywhere. And I decided to move the addition of pipelines and other stuff around mixed roles mode to different PR.

And yeah, in case that you are working on ST that will always have f.e. NodePools with mixed role, you will not use the NodePoolsConverter.

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

NodePoolsHandler, add NodePools creation to all testcases

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

checkstyle, javadoc, and fixups

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

fixup

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

fix http bridge tests

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

fixups to CC and NP ST classes

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

correct controllers and brokers

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

change label selectors, component names, etc.

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

fixups to KafkaST

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

multiple fixups

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

another fixes

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

another run, another fixes

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

another fixes

Signed-off-by: Lukas Kral <lukywill16@gmail.com>

fixups

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

/azp run upgrade

@im-konge
Copy link
Member Author

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge
Copy link
Member Author

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge im-konge marked this pull request as ready for review February 19, 2024 13:14
@im-konge
Copy link
Member Author

/azp run kraft-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

/azp run feature-gates-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

There are two test failures in feature-gates-regression pipeline:

[ERROR] Failures: 
[ERROR] io.strimzi.systemtest.cruisecontrol.CruiseControlST.testAutoCreationOfCruiseControlTopicsWithResources
[ERROR]   Run 1: CruiseControlST.testAutoCreationOfCruiseControlTopicsWithResources:144 
Expected: is <2>
     but: was <3>
[ERROR]   Run 2: CruiseControlST.testAutoCreationOfCruiseControlTopicsWithResources:144 
Expected: is <2>
     but: was <3>
[ERROR]   Run 3: CruiseControlST.testAutoCreationOfCruiseControlTopicsWithResources:144 
Expected: is <2>
     but: was <3>
[INFO] 
[ERROR] Errors: 
[ERROR] io.strimzi.systemtest.cruisecontrol.CruiseControlConfigurationST.testDeployAndUnDeployCruiseControl
[ERROR]   Run 1: CruiseControlConfigurationST.testDeployAndUnDeployCruiseControl:88 » Wait Timeout after 120000 ms waiting for Verify that Kafka contains CruiseControl Topics with related configuration.
[ERROR]   Run 2: CruiseControlConfigurationST.testDeployAndUnDeployCruiseControl:88 » Wait Timeout after 120000 ms waiting for Verify that Kafka contains CruiseControl Topics with related configuration.
[ERROR]   Run 3: CruiseControlConfigurationST.testDeployAndUnDeployCruiseControl:88 » Wait Timeout after 120000 ms waiting for Verify that Kafka contains CruiseControl Topics with related configuration.

which fails also on main branch and will be fixed in different PR, as they are not connected to my changes. I will open a different PR for that (as these tests need BTO due to checks inside, which is used only in the FG pipelines).

@im-konge im-konge merged commit ddbe437 into strimzi:main Feb 19, 2024
39 of 41 checks passed
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

4 participants