[WIP] Make variable expansion work for environment variables in k8s #12610

metlos · 2019-02-06T13:49:29Z

What does this PR do?

K8s does the expansion only if it already knows about the variable being
expanded.

This means we have to sort the environment variable list prior to sending
it to k8s in such a way that vars that reference others always follow the
referenced ones.

What issues does this PR fix or reference?

#12577

Release Notes

TODO

K8s does the expansion only if it already knows about the variable being expanded. This means we have to sort the environment variable list prior to sending it to k8s in such a way that vars that reference others always follow the referenced ones. Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos · 2019-02-06T14:11:19Z

ci-test

che-bot · 2019-02-06T17:39:56Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12610
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

+ a small clarification on a slightly non-obvious piece of code. Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos · 2019-02-07T09:53:26Z

ci-test

garagatyi

Looks good in terms of functionality but I would love seeing it a bit more simplifies to ease maintanance

...java/org/eclipse/che/workspace/infrastructure/kubernetes/provision/env/EnvVarsConverter.java

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

garagatyi · 2019-02-07T10:29:18Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+    }
+  }
+
+  static final class CycleException extends RuntimeException {


Can you elaborate on why we need to throw runtime exception instead of Infrastructure exception?

This is thrown from compare which must not throw a checked exception by contract.

I don't think it should be thrown from the compare method. Would rather refactor code to do dependencies and cycles processing in one method and call sort with the comparator in another

I don't even mean public methods, just restructure the code to respect single responsibility principle

I do the cycle detection lazily on purpose actually, because theoretically, it can be a lot of work. So I do it only once and only if necessary.

Depending on how the sorting algorithm is implemented it can find the offending cycle much sooner than we could if we went through the whole collection looking for them. This is actually just a bet that the more complex "traversal" of the list that the sorting algorithm is performing is going to bump into the possible offenders that happen to be located near the end of the input collection sooner than a linear traversal would.

As for the single responsibility principle, I guess I still don't see how this class breaks it. Could you please explain that in more detail?

It provides util method to sort env vars (1 responsibility) and it implements a comparing of items (2 responsibility)

As far as I understand there should be done quite the same work to find cyclic dependencies and sort env var.
I think CycleException solution is good enough.

garagatyi · 2019-02-07T10:58:50Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+  private EnvironmentVariablesSorter(Map<String, EnvVar> envVars) {
+    this.referenceMap = new HashMap<>(envVars.size());
+    for (Map.Entry<String, EnvVar> e : envVars.entrySet()) {
+      referenceMap.put(e.getKey(), findReferences(e.getValue()));


The same argument is used in both constructor and method and some work is done in the constructor and some in the method. It seems that it is kinda against the single responsibility principle.

So this class is a comparator, but it needs some initial set up. For the cycle detection to work, it needs to "see" the full list of environment variables to build the reference map eagerly so that it can be effectively used during the comparsions.

The static method is merely a helper to do the two things together - initialize the comparator and let it sort the environment variables.

At the same time I made the constructor private because it doesn't make sense to keep the instance around - the comparator is only valid for a concrete list of environment variables and cannot be reused for any other (because of the eager init of the internal state based on the list of env vars).

I could theoretically split this into 2 classes - the comparator and some new utitility class only exposing the single static method that would be the same as sort but I thought that would be an unnecessary overkill..

Since the class is not used as comparator elsewhere I do not think it can be treated as a comparator. The same could be achieved using private inner class without SRP violation

garagatyi · 2019-02-07T11:07:55Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+ *
+ * <p>This class makes sure that the environment variables are sorted.
+ */
+final class EnvironmentVariablesSorter implements Comparator<EnvVar> {


It doesn't seem like we need to use inheritance here. See example:

when Class1 IS something use inheritance

when Class1 USES something use composition

This class IS a comparator and it is USED as one. It is used by the single static method, called sort that happens to be located within the class itself. That doesn't make the class itself use a comparator.

Static methods, if not referencing to some static state, which is evil in its own regard, are themselves stateless and essentially independent of the class their placed in. Placing a static method into a class is basically namespacing, nothing more.

I would expect next usage from a class that is comparator like it is now:

List<EnvVar> ret = new ArrayList<>(envVars.values()); ret.sort(sorter);

where construction of the object and passing the same data to its constructor seems unneccessary.
Because of that, it seems to me that this util class should not serve as both comparator and util class that uses the comparator.

I also think that inheritance is not right thing there.
I see several possible ways here: Have EnvironmentVariablesSorter with EnvVarComparator implements Comparator<EnvVar> as an internal class. Then List<EnvVar> sort(Collections EnvVar> envVars) may be not static method, and tests would looks better. Like EnvVarsConverterTest would just test EnvVarsConverter functionality (override environment with machine provided, putting sorted value (but sorting is mocked)) and EnvironmentVariablesSorter would test different sort cases - like sort, do nothing if not needed, cyclic dependencies detection.

Or have EnvVarComparator only and then EnvVarsConverter may use it. The following example looks OK for me but then EnvVarsConverterTest would test two separate classes

// the env vars defined in our machine config take precedence over the ones already defined // in the container, if any container.getEnv().forEach(v -> envVars.putIfAbsent(v.getName(), v)); container.setEnv(new ArrayList<>(envVars.values())); // The environment variable interpolation only works if a variable that is referenced // .... container.getEnv().sort(new EnvVarComparator(container.getEnv()));

garagatyi · 2019-02-07T11:24:15Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+    // ok, the two env vars don't depend on each other, so we actually don't care too much about
+    // the order. We need to be consistent and total, though...
+
+    int sizeDiff = aTransRefs.size() - bTransRefs.size();


Since we are just trying to do dependency resolution it doesn't really make sense to sort in any other way. Adding other criteria just complicates the code. I would go with return 0.

You're right as long as we keep the rest of the code as is (-ish) and don't make this class reusable. If it could be used outside of the sort method, I would try to keep the implementation conform to the general contract of comparator - it needs to be consistent with equals and be total, which we'd be breaking with your suggestion.

I didn't mean to make it reusable, see my comments above. And sorry for the confusion. From what I saw it is unusual to sort variables in an alphabetical order

I'm still a little bit nervous about breaking the contract of comparable here. I didn't try it yet, but if we just used the dependency as the sorting criteria, the following is easily true:

a == b && b == c && a > c

(where == and > are defined in terms of our comparator)

E.g.

A="$(C)" B="blah" C="bleh"

I am no expert on the inner workings of the "stable, adaptive iterative mergesort" used by List.sort but I can easily imagine the above conditions producing different (and sometimes incorrect) order under different initial conditions (meaning the original order of the elements).

The comparator should by contract be "total" which amongst other things also means "transitive" as far as I looked. The above example breaks the transitivity assumption.

I'm not a sorting expert either, but the current approach seems overcomplicated. But the violation of sorting prerequisites can break the algorithm indeed, so should be present.

garagatyi · 2019-02-07T11:53:05Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+    if (recRefs == null) {
+      recRefs = new TreeSet<>(naturalOrder());
+      fillRecursiveReferences(variableName, referenceMap, recRefs);
+      recursiveReferenceMap.put(variableName, recRefs);


For me, it is not very clear that fillRecursiveReferences and recursiveReferenceMap.put are doing differnt job. Would be valuable to refactor code for simplification.

Would recursiveReferenceMap -> recursiveReferencesMap or maybe recursiveReferenceMap -> transitiveClosures help? I don't see how this could be simplified apart from renaming.

garagatyi · 2019-02-07T11:59:05Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+    return ret;
+  }
+
+  private static void fillRecursiveReferences(


This is not what compare was designed actually, maybe we should refactor to:

Prepare data - evaluation of dependencies and find cycles

Sort data

Which is exactly what is happening?

What I see is:

prepare data in the constructor

sort which includes

prepare data in the compare method

garagatyi · 2019-02-07T12:01:41Z

...clipse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariablesSorter.java

+  static final class CycleException extends RuntimeException {
+
+    CycleException(String variableName) {
+      super(format("Recursive definition of environment variable %s", variableName));


Can you elaborate on whether it is this a user-facing error?

The message is reused in the InfrastructureException thrown from sort

I believe we can simplify the message because not all the IDE users are that technical to understand the meaning of the message.

garagatyi · 2019-02-07T12:03:02Z

...pse/che/workspace/infrastructure/kubernetes/provision/env/EnvironmentVariableSorterTest.java

+  }
+
+  private static void testSingleValue(String value, Set<String> expected, String caseName) {
+    assertEquals(findReferences(var("name", value)), expected, caseName + ": sole");


TestNG has very useful feature for such cases - data provider.

A data provider could be used if I inverted the logic somewhat and supplied the individual test cases from https://github.com/eclipse/che/pull/12610/files/5b49b89ee272db490296942e2af6e68d9ae0e84e#diff-fe28a35161f1e8e64261b7c68612f0beR35 to a single test that would do the same thing as testSingleValue.

IMHO supplying the testcases as Object[][] would actually hurt the readability of the testcase, but that's just my taste. I am happy to rewrite it if you insist.

I do not insist! Just wondered whether you know about this capability.
I found reading this method hard. For example, still don't understand what is ": sole". Could you explain that to me?

sole as in solitary, meaning that the value is taken as is, unlike the rest of the tests where the value is (pre|ap)pended to. I'm modifying the "case name" so that the error message from testng mentions the concrete case that failed.

che-bot · 2019-02-07T13:43:24Z

Results of automated E2E tests of Eclipse Che Multiuser on OCP:
Build details
Test report
docker image: eclipseche/che-server:12610
https://github.com/orgs/eclipse/teams/eclipse-che-qa please check this report.

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos · 2019-02-08T14:17:53Z

Note that the current impl doesn't work at all, because we're trying to sort a list but what we actually have is an directed graph. A list sorting algorithm can never work on that structure, so I need to implement a proper topological sort...

Closing this and will open a new PR...

metlos requested review from garagatyi and sleshchenko as code owners February 6, 2019 13:49

metlos added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. team/platform labels Feb 6, 2019

che-bot added the kind/enhancement A feature request - must adhere to the feature request template. label Feb 6, 2019

metlos added 2 commits February 7, 2019 08:42

Fix the error message when variable directly defined using itself

6fc7de3

+ a small clarification on a slightly non-obvious piece of code. Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Improve the tests

5b49b89

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

garagatyi suggested changes Feb 7, 2019

View reviewed changes

metlos added 2 commits February 7, 2019 16:50

Add a test for the new converter behavior

fae6954

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

Removed obsolete code from the test

0ea22d4

Signed-off-by: Lukas Krejci <lkrejci@redhat.com>

metlos changed the title ~~Make variable expansion work for environment variables in k8s~~ [WIP] Make variable expansion work for environment variables in k8s Feb 8, 2019

metlos changed the title ~~[WIP] Make variable expansion work for environment variables in k8s~~ Make variable expansion work for environment variables in k8s Feb 8, 2019

metlos changed the title ~~Make variable expansion work for environment variables in k8s~~ [WIP] Make variable expansion work for environment variables in k8s Feb 8, 2019

metlos closed this Feb 8, 2019

metlos mentioned this pull request Feb 18, 2019

Make variable expansion work for environment variables in k8s #12689

Merged

1 task

[WIP] Make variable expansion work for environment variables in k8s #12610

[WIP] Make variable expansion work for environment variables in k8s #12610

Conversation

metlos commented Feb 6, 2019

What does this PR do?

What issues does this PR fix or reference?

Release Notes

metlos commented Feb 6, 2019

che-bot commented Feb 6, 2019

metlos commented Feb 7, 2019

garagatyi left a 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

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

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

metlos Feb 7, 2019 • 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

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

che-bot commented Feb 7, 2019

metlos commented Feb 8, 2019

metlos Feb 7, 2019 •

edited

Loading