OWLS 86115: Model in Image should not use operator generated configuration overrides #2285

lennyphan · 2021-03-27T17:05:20Z

NOTE: This pull is superseded by pull #2344

In this PR:

For Domain home source type MII, configuration overrides are generated using WDT model filter. Model filter implementation follows the same logic for generating situational configuration in introspectDomain.py's SitConfigGenerator. Domain home source types Domain-on-PV and Domain-in-Image will continue to use situational configuration overrides.
Added ability to run python based unit tests for model filter implementation.

Passed all MII integration tests except for the single test failure: oracle.weblogic.kubernetes.ItMiiDynamicUpdate.testMiiRemoveTarget which matches Integration Tests Nightly Results as of (03/26)

… variable

Merge latest updates from develop

rjeberhard · 2021-03-27T17:59:49Z

operator/src/main/java/oracle/kubernetes/operator/helpers/PodStepContext.java

    addEnvVarIfTrue(mockWls(), vars, "MOCK_WLS");
+
+    if (getDomainHomeSourceType() == DomainSourceType.FromModel) {
+      addEnvVar(vars, ServerEnvVars.FAIL_BOOT_ON_SITUATIONAL_CONFIG_ERROR, "false");


This is going to change our pod hash (weblogic.sha256) which means that any MII pod that is running when the operator is upgraded will be rolled. Can this instead be set in the pod start script (startServer.sh).

Just to clarify the scenario you're describing for my understanding, this is when upgrading the Operator from a previous MII domain that uses sit config overrides to this version which uses the model filter? The one place that uses this environment is in startNodeManager.sh (as a server startup argument) and I couldn't determine how we know the domain source type is 'FromModel' (MII). serverStart.sh does:

if [ -f /weblogic-operator/introspector/domainzip.secure ]; then
...
fi

I think this should work. Let me make the change and verify.

Right, a customer is using operator 3.1.4 and has a MII domain. The admin server and some number of managed servers are running. The customer upgrades to 3.2.0 containing your work. I would expect that pods do not roll simply because I upgrade the operator.

Thinking more deeply about this... The change to add the extra environment variable would have caused the existing pods to roll. I think that we have a workaround in that we can set this in startServer.sh. However, we likely have an earlier issue... Your change to not generate config overrides will mean that the generated domain home will be different. This change would also cause existing pods to roll, correct? If so, we should talk together with @jshum2479 and see if we can come up with a way to roll this out so that existing pods are not rolled or restarted, but newly created pods come up with the new pattern.

We can add logic in start script to reset the env when the DOMAIN_SOURCE_TYPE is FromModel.

It doesn't look like it'd hurt to leave it alone. It'd become a NO-OP.

The FAIL_BOOT_ON_SITUATIONAL_CONFIG_ERROR is for the core config override logic to failed to start the server if the override file is wrong, the start server script already have the logic to set it to true if the environment is not set, the env is for the case if user choose to override it.

I am not seeing a reason to set it for mii since we are using override and it will cause the roll because of the pod spec hash changed because the newly introduced environment variable.

The overall point is that there's no need to change or add the old env var setting in this pull. We don't care what it's set to in the new path...

in startNodeManager.sh, we set FAIL_BOOT_ON_SITUATIONAL_CONFIG_ERROR system property to 'true' by default. Managed servers fail to start/boot with the following error:

<Mar 29, 2021 8:22:23 PM GMT> <The SitConfigRequired configuration element is set to true, but no situational config files were found.>
<Mar 29, 2021 8:22:23 PM GMT> <Server failed. Reason: 141327>
<Mar 29, 2021 8:22:23 PM GMT>

The validation in WLS is in SituationalConfigManagerImpl:
if (isSitConfigRequired() && unexpiredFiles.isEmpty()) {
if (debugLogger.isDebugEnabled()) {
debugLogger.debug( "[SitConfig] isSitConfigRequired is true and no Sit Config Files exist. Failing boot.");
}
String l = ManagementLogger.logSituationalConfigRequired();
throw new ServiceFailureException(l);
}

So it looks like we should either set it to 'false' or not define it for MII.

The point is that we're not supporting custom or internal sit-cfg in MII regardless, so there won't be any sit-cfg that can fail, so the flag should have no effect. If removing it is safe (doesn't cause a roll), then feel free to remove it. Or feel free to leave it untouched.

rjeberhard · 2021-03-27T18:04:53Z

operator/src/main/resources/scripts/introspectDomain.py


    if istio_enabled == 'true':
+      name = nap.getName()
+      if name.startswith('http-') or name.startswith('tcp-') or name.startswith('tls-') or name.startswith('https-'):


Could this be fragile? The customer could have a nap name that starts with one of these prefixes. Do we have some earlier validation that ensures that the only names with these prefixes would be naps that were edited by our Istio functionality?

I could be wrong but I couldn't find any validation for naps that have these prefixes edited by our Istio functionality. @jshum2479 is there validation for these naps that I didn't see?

This function is to write out the topology in the introspector output (not config override), it is trying to format the name in istio ways. If the customer already have it that way and you skipped writing to the topology yaml, then they won't be setup in the pod correctly, perhaps changing the logic to not prepend the protocol again instead of returning.

operator/src/test/python/test_wdt_mii_filter.py

rjeberhard · 2021-03-27T18:13:24Z

Wow, @lennyphan, this is a ton of work. I think I'm going to need a "show-and-tell" since I don't know enough to review the filter and changes to the introspector script. I want to make sure that you've accounted for all of the situational configuration and Istio changes. I suspect that you have, but can't tell.

On the sit cfg environment variable, unfortunately, we've just been through learning and relearning why kind of changes cause operator upgrade bugs (this is where upgrading the operator causes problems to already running servers, including unexpected restarts). Anything that changes the generated pod model, including adding environment variables to the container, will do this. As noted, you could instead change the startServer.sh script that we inject.

I've delayed releasing 3.2.0 until we resolve one other upgrade bug, which will likely be done late Monday or Tuesday, so we could potentially get this change in :). I will be really happy to tell people about this improvement!

operator/src/test/java/oracle/kubernetes/operator/helpers/ConfigMapHelperTest.java

jshum2479 · 2021-03-29T14:54:05Z

operator/src/test/python/test_wdt_mii_filter.py

@@ -0,0 +1,243 @@
+import os
+import unittest
+import yaml


Which interpreter are we using here for package yaml?

I've been running with 2.7.1 and I believe that's the version running with WLST.

I think anyone who tries to build it needs to update the python to install the yaml package. I got import error - No module named yaml.

Thanks. Any suggestions on alternative? I know there is a yaml parser in WDT.

In addition to topology.yaml, we now also have topology.json. @ankedia added this to make it easier to consume the topology from scripts. Could you use this and jq?

I think this is the first time we introduce unit testing python into the test. WDT has it's own yaml parser, I don't think we want to introduce the dependency between two projects. If we agreed to build time python requirement, then we can just make it a requirement. @rjeberhard ?

Can the build or test suite verify that you have the right dependency installed? Can we verify that it's possible to build on both Oracle Linux and MacOS?

MacOS comes with Python 2.7 by default and it's possible, but a pain, to replace this with the Homebrew install of Python 3. What does this work require?

operator/src/main/resources/scripts/wdt_mii_filter.py

tbarnes-us · 2021-03-29T15:49:53Z

operator/src/main/resources/scripts/wdt_mii_filter.py

+
+import inspect
+import os
+import sys


General comment 1: I suspect the life-cycle and/or architecture documentation describes the propagation of internal sit-cfg. If so, then these docs should be updated.

General comment 2: Does this change lift any of the restrictions detailed in 'runtime-updates.md' for MII? If so, then the doc should be updated. Note that if the update includes adding support for shrinking a cluster we (a) may want to have integration tests before officially documenting support, (b) may want to make sure the documentation clearly states that JTA and JMS are not particularly amenable to such shrinks.

tbarnes-us · 2021-04-06T14:59:25Z

General comment: We should update the 'runtime update' user doc to reflect that this will lift some of the restrictions. For example sit-cfg conflict with logHome (and not documented it turns out for dataHome). Also perhaps other restrictions. Johnny, you, and I should probably read through the 'update' doc together to see what should be changed. We will need integration tests for each lifted restriction before we document support.

Merge latest changes from develop branch

Merge latest from develop

…netes-operator into OWLS-86115 merge from develop

…netes-operator into OWLS-86115 Pull latest from OWLS-86115 branch

… yaml

operator/src/main/resources/scripts/livenessProbe.sh

operator/src/main/resources/scripts/modelInImage.sh

operator/src/main/resources/scripts/model_wdt_mii_filter.py

operator/src/main/resources/scripts/startNodeManager.sh

operator/src/main/resources/scripts/startServer.sh

tbarnes-us · 2021-04-27T19:14:50Z

operator/src/main/resources/scripts/modelInImage.sh

-
  cp ${WDT_FILTER_JSON} ${WDT_ROOT}/lib/model_filters.json
  cp ${WDT_CREATE_FILTER} ${WDT_ROOT}/lib
+  cp ${WDT_MII_FILTER} ${WDT_ROOT}/lib


consider appending || trace SEVERE "cp failed" && exitOrLoop to each of the cp commands (fail-fast)

tbarnes-us

Way cool!

rjeberhard · 2021-04-27T19:47:36Z

@lennyphan, you need to adjust or recreate the PR to target the main branch.

rjeberhard · 2021-04-27T19:57:08Z

I merged your branch with main and pushed to OWLS-86115-main-merge. You can merge that branch into yours and then update the target of this PR.

rjeberhard · 2021-04-30T13:50:01Z

This PR was replaced.

lennyphan added 7 commits March 20, 2021 17:05

Use WDT filtering to generate config overrides for MII

7d55190

Merge from develop branch

1797cdc

Fix merge mistake

78257a8

Add additional unit tests for situational config override environment…

7438911

… variable

Fix for handling online dynamic update with sparse model

df14d4d

Merge remote-tracking branch 'origin/develop' into OWLS-86115

ad00e84

Merge latest updates from develop

cleanup debug output

7ec7810

lennyphan requested review from ddsharpe, jshum2479, rjeberhard, russgold and tbarnes-us March 27, 2021 17:05

rjeberhard reviewed Mar 27, 2021

View reviewed changes

operator/src/test/python/test_wdt_mii_filter.py Show resolved Hide resolved

tbarnes-us reviewed Mar 29, 2021

View reviewed changes

operator/src/test/java/oracle/kubernetes/operator/helpers/ConfigMapHelperTest.java Outdated Show resolved Hide resolved

jshum2479 reviewed Mar 29, 2021

View reviewed changes

tbarnes-us reviewed Mar 29, 2021

View reviewed changes

operator/src/main/resources/scripts/wdt_mii_filter.py Show resolved Hide resolved

tbarnes-us reviewed Mar 29, 2021

View reviewed changes

operator/src/main/resources/scripts/wdt_mii_filter.py Outdated Show resolved Hide resolved

tbarnes-us reviewed Mar 29, 2021

View reviewed changes

lennyphan added 8 commits April 7, 2021 00:28

Merge remote-tracking branch 'origin/develop' into OWLS-86115

eca7d2f

Merge latest changes from develop branch

First set of changes based on review feedback

2bcc31b

Merge remote-tracking branch 'origin/develop' into OWLS-86115

b4b4345

Merge latest from develop

Merge branch 'OWLS-86115' of https://github.com/oracle/weblogic-kuber…

48e4252

…netes-operator into OWLS-86115 merge from develop

Merge branch 'OWLS-86115' of https://github.com/oracle/weblogic-kuber…

c116a66

…netes-operator into OWLS-86115 Pull latest from OWLS-86115 branch

read domain name from topology, if available

00c29e5

add missing topology.yaml file

28f0e72

try using python ast package for reading dictionary values instead of…

4c8ec57

… yaml

tbarnes-us reviewed Apr 26, 2021

View reviewed changes

operator/src/main/resources/scripts/livenessProbe.sh Outdated Show resolved Hide resolved

tbarnes-us reviewed Apr 26, 2021

View reviewed changes

operator/src/main/resources/scripts/modelInImage.sh Outdated Show resolved Hide resolved

tbarnes-us reviewed Apr 26, 2021

View reviewed changes

operator/src/main/resources/scripts/modelInImage.sh Outdated Show resolved Hide resolved

tbarnes-us reviewed Apr 26, 2021

View reviewed changes

minor changes based on code review

5851ba6

This comment has been minimized.

Sign in to view

tbarnes-us reviewed Apr 27, 2021

View reviewed changes

tbarnes-us approved these changes Apr 27, 2021

View reviewed changes

Merge branch main

2cf4b3e

rjeberhard changed the base branch from develop to main April 27, 2021 20:02

tbarnes-us mentioned this pull request Apr 27, 2021

Recreate OWLS-86115 work #2344

Merged

rjeberhard closed this Apr 30, 2021

jshum2479 deleted the OWLS-86115 branch December 2, 2021 16:36

OWLS 86115: Model in Image should not use operator generated configuration overrides #2285

OWLS 86115: Model in Image should not use operator generated configuration overrides #2285

Uh oh!

Conversation

lennyphan commented Mar 27, 2021 • edited by tbarnes-us Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbarnes-us Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lennyphan Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jshum2479 Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjeberhard commented Mar 27, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tbarnes-us Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tbarnes-us commented Apr 6, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

tbarnes-us left a comment

Choose a reason for hiding this comment

Uh oh!

rjeberhard commented Apr 27, 2021

Uh oh!

lennyphan commented Mar 27, 2021 •

edited by tbarnes-us

Loading

tbarnes-us Mar 29, 2021 •

edited

Loading

lennyphan Mar 29, 2021 •

edited

Loading

jshum2479 Apr 30, 2021 •

edited

Loading

tbarnes-us Mar 29, 2021 •

edited

Loading