-
Notifications
You must be signed in to change notification settings - Fork 211
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
Network ports resolution between operator and wls runtime #2256
Conversation
…port values, there is a discrepancy between what the wls core is using and what is returned from wlst.
…x doesn't encounter user may already use 7002 in other context
if port == 0: | ||
return 7001 | ||
|
||
return server.getListenPort() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: could just 'return port'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Had a couple of comments/questions.
""" | ||
if server_template_sslports.has_key(server.getName()): | ||
configxml_ssl_port = server_template_sslports[server.getName()] | ||
if configxml_ssl_port is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if configxml_ssl_port is not None but has the value of 7002?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then configxml_ssl_port.getListenPort (actually it's a template) returns 7002, the server_template_listening_ports contains the actual value in the config.xml or None if no entry. Since we only care the case where there is no entry, I let the case to turn from the mbean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, since the server here is a template, WLS will treat the value as 7002, but mbean will likely return 8100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you are correct, even if 7002 is actually in the config.xml, it still returns 8100. I will change it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow this use case still does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a string comparison issue, fixed.
""" | ||
if server_template_listening_ports.has_key(server.getName()): | ||
port = server_template_listening_ports[server.getName()] | ||
if port is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if port is not None but has the value of 7001?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then server.getListenPort (actually it's a template) returns 7001, the server_template_listening_ports contains the actual value in the config.xml or None if no entry. Since we only care the case where there is no entry, I let the case to turn from the mbean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, since the server here is a template, WLS will treat the value as 7001, but mbean will likely return 7100.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somehow this use case still does not work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a string comparison issue, fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works now. thanks.
|
||
The difference occurs when a user specifies 7002 in the model | ||
or wlst offline for a server template when creating the domain, | ||
which results in an empty entry in the config.xml. When subsequently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an empty entry -> an empty entry or a 7002 entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
||
The difference occurs when a user specifies 7001 in the model | ||
or wlst offline for a server template when creating the domain, | ||
which results in an empty entry in the config.xml. When subsequently |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an empty entry -> an empty entry or a 7001 entry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
There are a couple of minor comments remaining from me - feel free to address them or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a couple of minor comments.
There are quite a number of failures in the integration test run in Jenkins: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4276/. Kicked off another run with the latest: https://build.weblogick8s.org:8443/job/weblogic-kubernetes-operator-kind-new/4278/. |
if listenPortProperty != firstListenPortProperty: | ||
self.addError(errorMsg.substitute(cluster=self.name(cluster), server1=self.name(firstServer), property=clusterListenPortProperty, value1=str(firstListenPortProperty), server2=self.name(server), value2=str(firstListenPortProperty))) | ||
return | ||
# def validateClusterServersListenPortProperty(self, cluster, errorMsg, clusterListenPortProperty): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove commented out code unless there is a specific need, e.g. it acts as documentation. In this case, it looks like it's just leftover after the other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -61,3 +61,5 @@ def trace(arg1,arg2='SENTINEL'): | |||
traceInner('FINE',arg1) | |||
else: | |||
traceInner(arg1,arg2) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is no substantive change to this file. If so, can you revert?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
I have come up with this so far and I think it is close, I have some commented code due to refactoring and will remove it later.