-
Notifications
You must be signed in to change notification settings - Fork 9
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
Fixes/kubernetes 1 #520
base: integration/pg
Are you sure you want to change the base?
Fixes/kubernetes 1 #520
Conversation
add k8s client dependency
Fix editorconfig style
…ture/k8s-dependency
…ture/k8s-dependency # Conflicts: # platform-controller/pom.xml
…tion and kubernetes client
change kubernetes client dependency
Migrate docker compose files to kompose
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.
I started to review the code and added some comments.
Please ensure that the code is documented correctly and remove the files that are not necessary as discussed during our meeting.
As already discussed during our meeting, it is very important that the files are formatted correctly. For example the PlatformController class as well as the pom.xml of the controller are very hard to review at the moment because a lot of changes are caused by different formattings.
- name: GITLAB_EMAIL | ||
value: oluoniyide@yahoo.com | ||
- name: GITLAB_TOKEN | ||
value: zRmVRQiodtu47Sj31Jmu |
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.
Please tell me that this is not your real token.... you should never upload a security token to git!!! 😱
k8s-config/elk/elasticsearch.yaml
Outdated
- name: "9200" | ||
port: 9200 | ||
targetPort: 9200 | ||
nodePort: 31064 |
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.
Are you sure that you need all 3 port definitions? I think the nodePort
exposes the elastic search port for external services which shouldn't be necessary.
@@ -672,7 +647,7 @@ protected void sendToCmdQueue(String address, byte command, byte data[], BasicPr | |||
/** | |||
* The controller overrides the super method because it does not need to check | |||
* for the leading hobbit id and delegates the command handling to the | |||
* {@link #receiveCommand(byte, byte[], String, String)} method. | |||
* method. |
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.
Why has the link to the receive command method removed?
|
||
import org.hobbit.controller.orchestration.objects.ClusterInfo; | ||
|
||
public interface ClusterManager { |
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.
If you have moved the interface to this new package, why did it lost all its Javadoc comments on the way? Please add them back.
@@ -0,0 +1,27 @@ | |||
package org.hobbit.controller.kubernetes.networkAttachmentDefinitionCustomResources; |
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.
This is not a good package name. It is way too long and it does not abide the typical naming conventions. Please fix this.
info.driverStatus(), info.experimentalBuild(), info.httpProxy(), info.httpsProxy(), info.id(), info.ipv4Forwarding(), | ||
info.images(), info.indexServerAddress(), info.initPath(), info.initSha1(), info.kernelMemory(), info.kernelVersion(), info.labels(), info.memTotal(), | ||
info.memoryLimit(), info.cpus(), info.eventsListener(), info.fileDescriptors(), info.goroutines(), info.name(), info.noProxy(), info.oomKillDisable(), info.operatingSystem(), | ||
info.osType(), info.systemStatus(), info.systemTime()); |
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.
A lot of these methods can not work since you removed the ability to throw exceptions. The problem is that these methods are simply not able to handle the exceptions in a meaningful way (i.e., apart from logging them). The only effect that it will have (in case an error occurs) is that you will cause a NullPointerException
.
The reason why all the methods in this class throw an exception is to let the piece of code that called a method know that there is a problem. So please add the possibility to throw Exceptions and make use of 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.
The same applies to changes in other classes.
|
||
import java.util.List; | ||
|
||
public interface ContainerManager<ReplicationController, Metrics> { |
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.
Again a class that has been moved and that "lost" a lot of its documentation. Please add the documentation back. It is really important to have the methods documented.
Apart from that, I am wondering why you have added <ReplicationController, Metrics>
to the class. What is the rational behind that? This would only make sense if for example the two metrics classes would share a common super type. As far as I know this is not the case.
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 addition to specific comments:
- there are a lot of whitespace changes mixed up with other commits, excess whitespace, unused empty files (
analysis-deployment.yaml
) - there are import reorganizations which are not in separate commits
- many commit messages do not describe the changes at all, or do not give any reason for the changes where it would be nice to have it
</module> |
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.
Should this file be just deleted and ignored?
@@ -1,5 +1,9 @@ | |||
FROM maven | |||
|
|||
LABEL maintainer="Hobbit Team" \ |
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.
Is this a thing?
LOGGER.error("Could not get cluster health status. ", e); | ||
} catch (InterruptedException e) { | ||
LOGGER.error("Interrupted. Could not get cluster health status. ", e); |
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.
Commit 384a430 has only this change and no explanation.
info.driverStatus(), info.experimentalBuild(), info.httpProxy(), info.httpsProxy(), info.id(), info.ipv4Forwarding(), | ||
info.images(), info.indexServerAddress(), info.initPath(), info.initSha1(), info.kernelMemory(), info.kernelVersion(), info.labels(), info.memTotal(), | ||
info.memoryLimit(), info.cpus(), info.eventsListener(), info.fileDescriptors(), info.goroutines(), info.name(), info.noProxy(), info.oomKillDisable(), info.operatingSystem(), | ||
info.osType(), info.systemStatus(), info.systemTime()); |
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.
The same applies to changes in other classes.
@@ -106,7 +107,7 @@ public void run() { | |||
} | |||
} | |||
} | |||
} catch (DockerException | InterruptedException e) { | |||
} catch (Exception e) { |
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.
Why change it to just Exception
?
} | ||
|
||
@Override | ||
public String startContainer(String imageName) { |
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.
This was marked as deprecated in ContainerManagerImpl
, why it's not marked as such here?
} | ||
|
||
@Override | ||
public String startContainer(String imageName, String[] command) { |
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.
This was marked as deprecated in ContainerManagerImpl
, why it's not marked as such here?
} | ||
|
||
@Override | ||
public String getContainerId(String name) { |
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.
This was marked as deprecated in ContainerManagerImpl
, why it's not marked as such here?
} | ||
|
||
@Override | ||
public String getContainerName(String containerId) { |
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.
This was marked as deprecated in ContainerManagerImpl
, why it's not marked as such here?
// Check whether there is a ':' in the remaining part of the image name | ||
return imageName.indexOf(':', pos) >= 0; | ||
} | ||
} |
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.
This method, as well as many others, should be in a shared parent/abstract class since it's identical to the one in ContainerManagerImpl
.
Kubernetes Client and configurations for Platform Controller