[WIP] Admin and canary blast radius control #504

sknot-rh · 2021-09-16T09:48:51Z

Use Templates from strimzi bundle to generate Admin/Canary Deployments

rareddy · 2021-09-16T19:14:58Z

@sknot-rh I might be missing something obvious why template-based loading now for these components? Do you expect to change these templates dynamically at runtime?

sknot-rh · 2021-09-17T06:24:40Z

Template ConfigMap is going to be part of the Strimzi bundle. So for each Strimzi version, there will be adequate Admin/Canary versions.

kornys

See comments regarding ST

systemtest/src/test/java/org/bf2/systemtest/integration/SmokeST.java

systemtest/src/test/java/org/bf2/systemtest/integration/AbstractST.java

rareddy · 2021-09-17T14:26:01Z

Template ConfigMap is going to be part of the Strimzi bundle.

Strimzi or kas-fleetshard bundle? from your code it looked like kas-fleetshard (also you hardcoded namespace for it, it is different in prod and staging).

So for each Strimzi version, there will be adequate Admin/Canary versions.
Can you tell what template differences are between versions you found that require this kind of template approach? I would have thought it would be the client libraries to access the Kafka cluster, which means the deployment image would change not the template itself.

IMO is half parameters can be made unique by {MK-NAME}-foo-bar, it may be simpler and have fewer properties to manage if the template itself is driven that way.

ppatierno · 2021-09-17T15:36:34Z

operator/src/main/java/org/bf2/operator/operands/Operand.java

+        writer.write(companionTemplates.getData().get(templateName));
+        writer.close();
+
+        DefaultOpenShiftClient dryOpenShiftClient = new DefaultOpenShiftClient();


I think that the templating we are using is not ok, because the Template resource is specific to OpenShift as you here have to use a specific OpenShift client. The FSO has to work on vanilla Kubernetes as well. Did I misunderstand anything?

The client is doing the substition localy (processLocally) so it is not really communicating with a server. I it just about converting template string to actual deployment.

sknot-rh · 2021-09-20T10:01:44Z

IMO is half parameters can be made unique by {MK-NAME}-foo-bar, it may be simpler and have fewer properties to manage if the template itself is driven that way.

From Template doc -

When using the ${PARAMETER_NAME} syntax only a single parameter reference is allowed and leading and trailing characters are not permitted.

kornys

Please fix my comment

systemtest/src/main/java/org/bf2/systemtest/operator/FleetShardOperatorManager.java

kornys

LGTM from ST POV

rareddy · 2021-09-21T20:35:37Z

operator/src/main/java/org/bf2/operator/operands/AbstractCanary.java


    @Override
    public void createOrUpdate(ManagedKafka managedKafka) {
-        Deployment current = cachedDeployment(managedKafka);
-        Deployment deployment = deploymentFrom(managedKafka, current);
+        ConfigMap companionTemplates = configMapResource(kubernetesClient.getNamespace(), "companion-templates-config-map").get();


From my earlier question on where the templates are coming from, here it indicates that this configmap is part of same namespace as the FSO, and may be created as part of the operator deployment or created by someone else during the install. My question

Where does this template originate from? and how it gets deployed as configmap in FSO namespace? you mentioned Strimzi but I did not find any references on quick search.

What benefit does it give us using the template form? I see you still deploying a single deployment of either a canary or admin server.

Sorry for the unclear explanation :( (there is also a epic brief you can read)
1.

Where does this template originate from?

We decided to use a Template to specify as much as possible configuration to Deployment. The main motivation is to use corresponding Admin/Canary images with the new Strimzi bundle. So if you upgrade Strimzi (new bundle will be used) the Admin/Canary deployments will be regenerated and the correct image used.

and how it gets deployed as ConfigMap in FSO namespace?

IIUC it will be part of the Strimzi bundle. So when the bundle is created, this ConfigMap should be created as well.

What benefit does it give us using the template form?

Admin/Canary is bonded to ManagedKafka resource and some configuration is different. For example name of the name of MK resource does affect `bootstrap-servers` address etc. So you use placeholders in Template to generate MK resource-related Admin/Canary deployments.

I see you still deploying a single deployment of either a canary or admin server.

Yeah, but each Admin/Canary deployment is specific to a MK resource, so the deployments are different.

sorry, I may be missed EpicBreif on it, if you can give link I will take a look.

So the new strimzi bundle will have the configmap with the image details backed into the deployment template and FSO need not to be changed every time there is a change to canary or admin servers. If there is a change then one needs to create a new Strimzi bundle for it. Is that accurate?

Yes, that is right. EB

rareddy · 2021-09-21T20:39:29Z

operator/src/main/java/org/bf2/operator/operands/AdminServer.java

+    private Map<String, String> buildParameters(ManagedKafka managedKafka) {
+        List<Parameter> parameters = new ArrayList<>();
+
+        addParameter(parameters, "KAFKA_ADMIN_SERVER_APP", adminServerName(managedKafka));


Maybe these prefixes like KAFKA_ADMIN or CANRY_ are verbose? unless you trying to put different containers on single pod. They do not help with version-specific anyways if we are conceiving of multiple admin server deployments per pod

This is the name of the placeholder, not ENV.

what I am going after is you can reduce the number of tokens overall to be set, if you make then not use prefixes, and reuse the properties. Like the name is used to distinguish an attribute, you can reuse name everywhere then defining a new one for each occurrence. But of course, you would need to update the bundle for this.

rareddy · 2021-09-24T14:10:29Z

operator/src/main/java/org/bf2/operator/operands/AdminServer.java

+            deployment.getSpec().getTemplate().getSpec().getContainers().get(0).getEnv().add(new EnvVarBuilder().withName("KAFKA_ADMIN_OAUTH_JWKS_ENDPOINT_URI").withValue(oauth.getJwksEndpointURI()).build());
+            deployment.getSpec().getTemplate().getSpec().getContainers().get(0).getEnv().add(new EnvVarBuilder().withName("KAFKA_ADMIN_OAUTH_VALID_ISSUER_URI").withValue(oauth.getValidIssuerEndpointURI()).build());
+            deployment.getSpec().getTemplate().getSpec().getContainers().get(0).getEnv().add(new EnvVarBuilder().withName("KAFKA_ADMIN_OAUTH_TOKEN_ENDPOINT_URI").withValue(oauth.getTokenEndpointURI()).build());
+        }


Conditional code like this is where template processing falls down IMO, once you say you know the structure of the template you created a strong coupling between them and chances of breaking it during the runtime. Ideally, you shouldn't know what is in there, just text replace then deploy, but oh well!

operator/src/main/java/org/bf2/operator/operands/AdminServer.java

operator/src/main/java/org/bf2/operator/operands/dev/DevelopmentAdminServer.java

operator/src/main/java/org/bf2/operator/operands/Canary.java

Signed-off-by: Stanislav Knot <[email protected]>

shawkins · 2022-02-01T02:47:36Z

Closing in favor of #643

github-actions bot added the operator changes related to operator label Sep 16, 2021

github-actions bot added the test Test related changes label Sep 17, 2021

kornys requested changes Sep 17, 2021

View reviewed changes

systemtest/src/test/java/org/bf2/systemtest/integration/SmokeST.java Outdated Show resolved Hide resolved

systemtest/src/test/java/org/bf2/systemtest/integration/AbstractST.java Outdated Show resolved Hide resolved

ppatierno requested changes Sep 17, 2021

View reviewed changes

sknot-rh requested review from ppatierno and kornys September 21, 2021 10:13

kornys requested changes Sep 21, 2021

View reviewed changes

systemtest/src/main/java/org/bf2/systemtest/operator/FleetShardOperatorManager.java Outdated Show resolved Hide resolved

kornys approved these changes Sep 21, 2021

View reviewed changes

rareddy reviewed Sep 21, 2021

View reviewed changes

rareddy reviewed Sep 24, 2021

View reviewed changes

MikeEdgar reviewed Sep 24, 2021

View reviewed changes

k-wall added the DO NOT MERGE label Sep 28, 2021

sknot-rh added 10 commits October 26, 2021 10:29

Create template to generate deployment

2c1dfb5

Signed-off-by: Stanislav Knot <[email protected]>

workaround for templates

5c178a8

Signed-off-by: Stanislav Knot <[email protected]>

fix test

82c4cf2

Signed-off-by: Stanislav Knot <[email protected]>

ST

f635807

Signed-off-by: Stanislav Knot <[email protected]>

rebase

68702bf

Signed-off-by: Stanislav Knot <[email protected]>

progress

0d344d2

Signed-off-by: Stanislav Knot <[email protected]>

DK comments

279c318

Signed-off-by: Stanislav Knot <[email protected]>

Michael's comments

deae2cc

Signed-off-by: Stanislav Knot <[email protected]>

rebase

de45fd0

Signed-off-by: Stanislav Knot <[email protected]>

rebase

e7d83a6

Signed-off-by: Stanislav Knot <[email protected]>

shawkins closed this Feb 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Admin and canary blast radius control #504

[WIP] Admin and canary blast radius control #504

sknot-rh commented Sep 16, 2021

rareddy commented Sep 16, 2021

sknot-rh commented Sep 17, 2021

kornys left a comment

rareddy commented Sep 17, 2021 •

edited

Loading

ppatierno Sep 17, 2021

sknot-rh Sep 18, 2021

sknot-rh commented Sep 20, 2021

kornys left a comment

kornys left a comment

rareddy Sep 21, 2021

sknot-rh Sep 24, 2021 •

edited

Loading

rareddy Sep 24, 2021

sknot-rh Sep 27, 2021

rareddy Sep 21, 2021

sknot-rh Sep 24, 2021

rareddy Sep 24, 2021

rareddy Sep 24, 2021

shawkins commented Feb 1, 2022

[WIP] Admin and canary blast radius control #504

[WIP] Admin and canary blast radius control #504

Conversation

sknot-rh commented Sep 16, 2021

rareddy commented Sep 16, 2021

sknot-rh commented Sep 17, 2021

kornys left a comment

Choose a reason for hiding this comment

rareddy commented Sep 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknot-rh commented Sep 20, 2021

kornys left a comment

Choose a reason for hiding this comment

kornys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sknot-rh Sep 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shawkins commented Feb 1, 2022

rareddy commented Sep 17, 2021 •

edited

Loading

sknot-rh Sep 24, 2021 •

edited

Loading