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.
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.
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.
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.
| * 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.
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.
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.
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.
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.
The same applies to changes in other classes.
|
|
||
| import java.util.List; | ||
|
|
||
| public interface ContainerManager<ReplicationController, Metrics> { |
There was a problem hiding this comment.
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.
denkv
left a comment
denkv
left a comment
There was a problem hiding this comment.
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
| <orderEntry type="library" scope="TEST" name="Maven: junit:junit-dep:4.10" level="project" /> | ||
| </component> | ||
| </module> No newline at end of file | ||
| </module> |
There was a problem hiding this comment.
Should this file be just deleted and ignored?
| @@ -1,5 +1,9 @@ | |||
| FROM maven | |||
|
|
|||
| LABEL maintainer="Hobbit Team" \ | |||
| } catch (Exception e) { | ||
| 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.
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.
The same applies to changes in other classes.
| } | ||
| } | ||
| } catch (DockerException | InterruptedException e) { | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Why change it to just Exception?
| } | ||
|
|
||
| @Override | ||
| public String startContainer(String imageName) { |
There was a problem hiding this comment.
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.
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.
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.
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.
This method, as well as many others, should be in a shared parent/abstract class since it's identical to the one in ContainerManagerImpl.
7 participants
Kubernetes Client and configurations for Platform Controller