Skip to content

Nginx mii new it #1614

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

Merged
merged 22 commits into from
May 6, 2020
Merged

Nginx mii new it #1614

merged 22 commits into from
May 6, 2020

Conversation

xiancao
Copy link
Contributor

@xiancao xiancao commented May 2, 2020

This PR is to add Nginx ingress controller functionality in new integration test and use Nginx to access the sample apps in the model-in-image domain.

@xiancao xiancao requested review from vanajamukkara and sankarpn May 4, 2020 16:26
@xiancao xiancao requested review from markxnelson and rjeberhard May 4, 2020 21:38
Copy link
Member

@markxnelson markxnelson left a comment

Choose a reason for hiding this comment

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

a few small things to fix, overall very nice, thank you

private final int replicaCount = 2;

/**
* Install operator and Nginx.
Copy link
Member

Choose a reason for hiding this comment

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

NGINX - spelling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

.and().with().pollInterval(10, SECONDS)
.atMost(5, MINUTES).await();

// get a new unique operator namespace
Copy link
Member

Choose a reason for hiding this comment

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

comment and log message don't match what you are doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

assertNotNull(namespaces.get(1), "Namespace list is null");
domainNamespace = namespaces.get(1);

// get a new unique Nginx namespace
Copy link
Member

Choose a reason for hiding this comment

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

spelling - NGINX - will not comment on any more, but this applies to all of them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

// create image with model files
String miiImage = createImageAndVerify();

// push the image to OCIR to make the test work in multi node cluster
Copy link
Member

Choose a reason for hiding this comment

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

OCIR -> registry
you don't know it will be OCIR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

logger.info("Check managed server service {0} is created in namespace {1}",
managedServerPrefix + i, domainNamespace);
checkServiceCreated(managedServerPrefix + i);
}
Copy link
Member

Choose a reason for hiding this comment

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

very nice test! thank you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you!

} else {
try {
// sometimes the pod is not ready even the condition check is ready, sleep a little bit
Thread.sleep(100);
Copy link
Member

Choose a reason for hiding this comment

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

try { 
  Thread.sleep(100); 
} catch (InterruptedException ignore) {
  // ignored intentionally 
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
});
} catch (Exception e) {
logger.info("Got exceptions while running command: " + curlCmd);
Copy link
Member

Choose a reason for hiding this comment

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

singular

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}
}

// after the max iterations, check if any managedserver value is false
Copy link
Member

Choose a reason for hiding this comment

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

managed server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

});

// final check if any managed server value is false
return !managedServers.containsValue(false);
Copy link
Member

Choose a reason for hiding this comment

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

feels odd to do the same calculation over and over
why not do it once and store the result in a var

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

*/
public static int getNextFreePort(int from, int to) {
int port;
for (port = from; port < to; port++) {
Copy link
Member

Choose a reason for hiding this comment

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

either fix your javadoc, or use <=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@xiancao
Copy link
Contributor Author

xiancao commented May 6, 2020

@xiancao
Copy link
Contributor Author

xiancao commented May 6, 2020

In this run, https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-model-in-image-tests-10/, ItMiiDomain passed. However ItSimpleNginxValidation did not run since NGINX pod stuck in the pending state and BeforeAll failed.

@xiancao
Copy link
Contributor Author

xiancao commented May 6, 2020

@xiancao xiancao changed the title WIP: Nginx mii new it Nginx mii new it May 6, 2020
Copy link
Member

@sankarpn sankarpn left a comment

Choose a reason for hiding this comment

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

It looks good.

@rjeberhard rjeberhard merged commit 2243469 into develop May 6, 2020
}

// check that NGINX can access the sample apps from all managed servers in the domain
String curlCmd = String.format("curl --silent --noproxy '*' -H 'host: %s' http://%s:%s/sample-war/index.jsp",
Copy link
Contributor

Choose a reason for hiding this comment

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

using --silent here, when curl fails I think you can't see the error message why its failing by using --silent..use --silent along with --showerror and can you make the curl fail and see if you get the actual error message printed to debug..

/**
* Install WebLogic operator and wait until the operator pod is ready.
*/
private static void installAndVerifyOperator() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify, I am not working on re-organizing the common methods..I will be creating a basic domain image for MII in extension class which can be used by It classes.

}
});
} catch (Exception e) {
logger.info("Got exception while running command: " + curlCmd);
Copy link
Contributor

Choose a reason for hiding this comment

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

log error message as well..result.stderr and result.stdout

@xiancao xiancao deleted the nginx-mii-new-it branch May 7, 2020 15:12
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.

5 participants