Use Deployments instead of Pods in Kubernetes/OpenShift infra#10021
Use Deployments instead of Pods in Kubernetes/OpenShift infra#10021amisevsk merged 1 commit intoeclipse-che:masterfrom
Conversation
amisevsk
added
kind/enhancement
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@garagatyi, @sleshchenko I'm very open to suggestions on improving how this is done -- I'm still relatively unfamiliar with this section of the code base. Also, note that there are a few caveats with this PR in its current state:
@eivantsov Regarding documentation, I believe that the way I've implemented this means that all changes should be internal. We are basically just wrapping |
|
@amisevsk keep in mind openshiftio/openshift.io#875 |
|
I've added a commit to use @l0rd I'm a little unclear on the discussion in the issue linked -- replica sets are created by deployments to manage the pods. With the just added commit, I suppose we are back on replica sets as a side effect of using deployments. However, the suggestion to move to deployments seems strange, since that will result in the creation of replicasets anyways. The general structure, as I understand it, is DeploymentConfigs are what seems to be causing us issues given the version of kubernetes-client we are using. |
There was a problem hiding this comment.
Generally looks good 👍
I think using of DeploymentConfig is a bit overhead in Che use case because Che Server does deployment management function and using Deployments doesn't give many benefits =) But looks like we have not a choice. Maybe there is another way to mark Che Servers Pods as long-living?
So, I would say that extending KubernetesPods by OpenShiftDeployments is not the best way (maybe easiest but not the best :) ) to solve this issue.
I want to propose to you consider the following architecture:
You can add new abstraction like MachinePods, to create pods for machines (by creation pods or using deployment configs). It would look like:
MachinePods:
-create
-watch
-exec
-delete
...
KubernetesPods and OpenshiftDeploymentsConfigs would encapsulate operations with fabric8 k8s/os clients.
Then K8s/OS infrastructure will use MachinePods to work with machine pods and there will be two implementations - Pods based and DeploymentsConfigs based.
What could be better in this approach - deployment configs operation with fabric8 client would be separated from logic with adaption pods by DeploymentsConfig.
P.S. I would say that MachinePods is just topic to discuss but not requirement or recommendation.
| .deployments() | ||
| .inNamespace(namespace) | ||
| .withName(name); | ||
| // This step is necessary becuase the name string can refer to either a DeploymentConfig or |
| if (pods.isEmpty()) { | ||
| throw new InfrastructureException(String.format("Failed to find pod with name %s", name)); | ||
| } else if (pods.size() > 1) { | ||
| throw new InfrastructureException( |
There was a problem hiding this comment.
In this case, there can be some period when it's not possible to do anything with pod while restarting of pod (when there is one terminating and one starting/started). Maybe it makes sense to check status here
There was a problem hiding this comment.
Could you go into this scenario a little more? As I see it, there shouldn't be a situation in which multiple pods are within the same deployment, and so if that does occur it suggests an error state somewhere along the way.
I'm trying to work something out where we check for the rolling state, but it seems like it would add quite a bit of logic, so for clarity's sake I want to understand the motivation.
Me too honestly :-) Let's try to figure this out with the issue author in the next days
Have you been using v3.1.12? eclipse-che/che-dependencies@14a8533
If that's not the good choice we should not do that. |
|
@sleshchenko I agree that at this moment Deployments are somewhat of an unnecessary overhead from a Che server perspective, but I still think it makes more sense than using bare pods, as on OpenShift (from my understanding, at least) bare Pods are generally intended for short-running (i.e. terminating) jobs. The solution to making Pods long living would be to set Regardless, that's not a settled matter and I believe we're still discussing it, so there may be another way or workaround I haven't considered. Regarding the MachinePods abstraction, I'm happy to try to implement something like this, but I worry that it will only add confusion. I want to keep the underlying implementation details as Pods to avoid separating the OpenShift infra from the Kubernetes infra too much, so inserting this abstraction feels like it would be somewhat like just renaming KubernetesPods to WorkspacePods. Alternatively we could separate KubernetesPods and OpenShiftDeploymentConfigs entirely and connect them only via interface WorkspacePods, but then there is a lot of duplicated code between the two -- much of the functionality in OpenShiftDeploymentConfigs is just a simple indirection to translate Deployment name into Pod name (which to me makes sense, because fundamentally workspaces are Pods at the end of the day). I'll investigate it more when I have a bit of time. @l0rd I agree, I'd like to get this right so that we don't have a nightmare on our hands later. Regarding kubernetes-client, this is all with 3.1.12 at the moment; 3.2.0 was released a few days ago but I haven't checked if that changes anything. If a later version of kubernetes-client does change to using the
Assuming kubernetes-client would maintain its internal API upon the switch, the last two points do not seem to be large issues. From eyeballing the API docs for the two versions of Deployment, it does not appear we are using deployments in any way that is not supported in |
I guess it's not the problem to inject KubernetesPods into OpenShiftDeploymentConfigs, no need to duplicate code. Deployment based implementation of exec method for WorkspacePods may look like So, this class would contain only logic to related to mapping Pod <> DeploymentConfig. While I was writing this message I've remembered that we use KubernetesPods in PVCSubpathHelper, I think we really don't need deployment config for precreate/remove jobs. So, with extending it is impossible to create job anymore, with MachinePods/WorkspacePods is still possible to inject KubernetesPods directly and create job pod. |
garagatyi
left a comment
garagatyi
left a comment
There was a problem hiding this comment.
Overall looks good. I believe that this PR needs to be carefully tested since it changes very basic operations and something like what @sleshchenko described with PVC might be broken.
We should also think whether new WSMaster can pick up old workspace pods after update of Che on OSIO with this changes. As far as I know, we don't want to stop workspaces on each update of Che, so we should be sure that this change either doesn't incorporate incompatibility or require workspaces stopping.
I also don't like complexity and duplication that this PR adds, but don't see any proper alternative. Looks like we reached the point when we can't keep the code more or less simple.
Regarding the last comment from @sleshchenko about PVC: since Angel used k8s deployments instead of Openshift DeploymentConfigs (recent change that might not be reflected in the PR description) we can consider moving this code to k8s infrastructure and add an ability to use Deployments, bare pods, jobs for different actions, like PVC subpath creation. It might do Openshift infrastructure code that simple again but would extend k8s infrastructure instead.
Apart from that maybe we should think what we might do if support of all the entities from k8s model is needed and how we would handle that. In that case, we would have a direct way of creation of Jobs, bare pods, deployments, etc.
|
So after discussion today, two significant issues have come up:
Also, the fact that we are now using Deployments instead of Pods (i.e. still completely Kubernetes-side implementations) makes the structure of this PR make significantly less sense. I think something along the lines of what @sleshchenko suggested (introducing a new @garagatyi @sleshchenko WDYT? |
amisevsk
changed the title
|
I think we don't need an ability to configure the type of pods scheduling (bare, deployment, etc). Let's stick to one solution unless we actually need to support several options. |
amisevsk
force-pushed
the
openshift-infra-use-deployments
branch
from
d062996 to
1ea38a6
Compare
|
@garagatyi @sleshchenko I've redone most of this PR. The changes as of now are:
Any thoughts / suggestions? |
|
@amisevsk can you resolve conflicts in the PR? |
garagatyi
left a comment
garagatyi
left a comment
There was a problem hiding this comment.
@amisevsk you did a great job in making this change small and convenient for the Che master update! And the drawback of this is that code maintainability might be significantly affected.
It seems we need to think about better separation/abstraction not to make code too complex.
@amisevsk @sleshchenko WDYT?
| putLabel(pod, CHE_WORKSPACE_ID_LABEL, workspaceId); | ||
| ObjectMeta metadata = pod.getMetadata(); | ||
| PodSpec podSpec = pod.getSpec(); | ||
| podSpec.setRestartPolicy("Always"); // Only allowable value |
There was a problem hiding this comment.
Should we restart a pod if a container exits with the exit code 0? I'm not sure. It is kinda a situation that should not happen, right? So maybe it makes sense to use OnFailure? I don't have a strong opinion on that, just thinking about such a use case.
There was a problem hiding this comment.
Sadly, the only supported option in Deployments (at least on OpenShift) seems to be "Always". Setting it to "OnFailure" causes the POST request to be rejected. For what it's worth however, I manually failed a pod, and the workspace was detected as not running by the server (due to wsagent crashing) quickly and it showed a fairly understandable prompt to restart the workspace.
While the deployment does recreate a Pod that does nothing, it doesn't seem to have a significant negative impact. Hopefully in the future we'll be able to leverage the built in restart functionality to deal with failing workspaces through Kubernetes/OpenShift directly.
| * @return the created pod | ||
| * @throws InfrastructureException when any error occurs | ||
| */ | ||
| public Pod createTemporary(Pod pod) throws InfrastructureException { |
There was a problem hiding this comment.
Maybe we should use some kubernetes native vocabulary not to confuse with a new concept of a temporary pod?
There was a problem hiding this comment.
I agree, the naming in this class has become quite confusing. There's now somewhat of a split role in KubernetesDeployments, as its becoming more of a utility class for managing Pod resources (both through bare pods and deployments) than a simple wrapper around Pods.
In picking this name (which I agree is not very good), I was trying to choose something that would at least not add to the confusion -- calling it createJob is deceptive as it currently doesn't create jobs, and calling it createPod is confusing since both create methods result in the creation of pods. I'm starting to lean towards finding a sensible non-kubernetes vocabulary way of describing what this class does, and renaming it away from KubernetesDeployments
Perhaps
KubernetesDeployments -> KubernetesComputeResources
.create() -> .createNotTerminating()
.createTemporary() -> .createTerminating()
This would at least bring it in line with the language used in describing resource quotas. WDYT?
There was a problem hiding this comment.
I would say that will be created bare pod terminating or not depends only on infrastructure configuration.
Maybe #deploy(Pod) and create(Pod) will make more clear that one method creates pod directly while another one wraps pod into Deployment. Then we have to document which limitations have creation bare pod.
Another way to make the clear purpose of create(Pod) method may be using something like that create(Pod pod, String[] command). So it would be clear that this method should be used only if a user wants to execute a one-time task. We would even be able to move PVCSubPathHelper#execute(String, String[], String...) to KubernetesDeployment, so not only PVCSubPathHelper would be able to easily execute a command (submit task).
Guys. WHYT? Does it make sense?
| } | ||
|
|
||
| /** | ||
| * Create a terminating pod that is not part of a Deployment. |
There was a problem hiding this comment.
The class is called Deployment but methods create a pod that is not a part of the deployment. I think it may be a bit confusing.
There was a problem hiding this comment.
I agree, see the comment above about renaming to create[Not]Terminating() to better reflect the intended purpose of this class.
| Pod actualPod = podResource.get(); | ||
| if (actualPod == null) { | ||
| throw new InfrastructureException("Specified pod " + name + " doesn't exist"); | ||
| throw new InfrastructureException("Specified pod " + podName + " doesn't exist"); |
There was a problem hiding this comment.
There is a situation when deployment was specified and the message may be confusing because it says that podName was specified while it wasn't.
There was a problem hiding this comment.
Yeah this is a tricky one. One of the things I'm not quite happy with in this PR is that as a result of trying to not affect the rest of the internal workspaces representation (as Pods are used extensively), I've had to keep the underlying way of running pods invisible to che-server. While this allows most of Che to work without any modification, within this class, it makes it confusing as you point out, as it's possible to call this method with a deployment name.
If this message were to be logged, the two causes are
- The method is called with a deployment name, and
getPodName()succeeds, returning the name of a running pod controlled by the specified deployment, and that running pod terminates between the call togetPodName()and thisget()call. In this case, the log message should be something along the lines ofNo pods in Deployment [name] found - The method is called with a pod name directly (i.e. there is no deployment), and the pod that we
get()in thegetPodName()method terminates before we reach this point. In this case ideally we would log the message currently there.
Does this make sense, or can we make it more understandable? This check should really never fail, as getPodName always calls get() on the same PodResource and throws an error earlier if null is encountered.
There was a problem hiding this comment.
I've modified the logging to reflect the above, leveraging the fact that if name.equals(podName) then name refers to a bare pod. It does make the already-long error handling block in these methods longer, however. Let me know what you think.
|
@garagatyi Regarding the abstraction and separation question, I'm definitely open to moving things around if it makes more sense. For me, since so much of the functionality is shared between the two ways of launching pods (deployments vs. bare pods), it makes sense to keep everything in one class rather than have imports and calls across multiple classes. In reality, the only methods that care whether we are running in a bare pod or a deployment are the create() and delete() methods, as they have to do something different. Most of the other classes operate / interact with the pods themselves directly. Additionally, keeping everything together like this ensures that we can pick up old workspaces running in bare pods and handle them transparently if che server is hot updated. Do you have any ideas on a cleaner separation between the two? |
sleshchenko
left a comment
sleshchenko
left a comment
There was a problem hiding this comment.
About separation, I think it may do code cleaner if we would have two separate classes for KubernetesDeployments and KubernetesPods, then KubernetesDeployments would have only logic related to managing deployments, mapping deployment to Pod and reuse method of KubernetesPods to work with pods.
Additionally, keeping everything together like this ensures that we can pick up old workspaces running in bare pods and handle them transparently if che server is hot updated.
I think it's possible to do the same with two classes. The only one thing that should be implemented in KubernetesDeployment - try to pick up bare pod if pod managed by deployment was not found while pod to deployment mapping. And it may be removed after few releases.
| * @return the created pod | ||
| * @throws InfrastructureException when any error occurs | ||
| */ | ||
| public Pod createTemporary(Pod pod) throws InfrastructureException { |
There was a problem hiding this comment.
I would say that will be created bare pod terminating or not depends only on infrastructure configuration.
Maybe #deploy(Pod) and create(Pod) will make more clear that one method creates pod directly while another one wraps pod into Deployment. Then we have to document which limitations have creation bare pod.
Another way to make the clear purpose of create(Pod) method may be using something like that create(Pod pod, String[] command). So it would be clear that this method should be used only if a user wants to execute a one-time task. We would even be able to move PVCSubPathHelper#execute(String, String[], String...) to KubernetesDeployment, so not only PVCSubPathHelper would be able to easily execute a command (submit task).
Guys. WHYT? Does it make sense?
|
Right now I would go with the next solution:
Seems that we don't have enough certainty about the naming, so let's strip down the amount of work to apply to this PR and continue improving code in future when that certainty or necessity comes. |
|
Alright I have updated the PR with the method name changes suggested. I think that the documentation still makes sense but I may have missed something in the refactor. Let me know if there is anything else that needs to be changed before a rebase. @sleshchenko I like your idea of splitting it into two classes, so I would like to create an issue after this is merged where we can discuss it further and settle on a design choice before starting implementation. Does that sound good to you? I'm mostly concerned around increased confusion since a |
garagatyi
left a comment
garagatyi
left a comment
There was a problem hiding this comment.
PR looks good. Please, run CI tests before merging it
Modifies KubernetesPods to create workspaces with Deployments instead of bare Pods. Changes: - Rename KubernetesPods -> KubernetesDeployments to indicate change - Add method to KubernetesDeployments to allow launching short jobs (e.g. PVC Cleanup) - Modify create method to create Deployments - Modify delete method to support deleting both Deployment-based and Pod-based workspaces (allowing updating che-server without shutting down all workspaces) - changes apply to Kubernetes and OpenShift infrastructures. Signed-off-by: Angel Misevski <amisevsk@redhat.com>
amisevsk
force-pushed
the
openshift-infra-use-deployments
branch
from
ca5447c to
68bcb12
Compare
|
ci-test |
1 similar comment
|
ci-test |
|
ci-test build report: |
|
In the case where a pod has tens of thousands of files, it can take several minutes for the chown to happen on mount. The deploymentconfig here will need to take this into account and hopefully not terminate the pod spin up prematurely. |
|
@mmclanerh does |
|
@amisevsk when you are going to merge do not forget to remove [WIP] from description since PR title would be used for the changes log |
amisevsk
changed the title
benoitf
removed
the
status/code-review
|
I'd be great to update docs too. |
|
@eivantsov I'm happy to take a pass over the docs to reflect this change. Are there any sections of the documentation you think I should focus on (e.g. Kube / OS admin guides?) |
|
@amisevsk I think I will start a branch and update deployment images that explain what objects we create, and we can go from there. |
8 participants
What does this PR do?
Changes OpenShift infrastucture to use
DeploymentConfigs instead ofPods for workspace machines.What issues does this PR fix or reference?
This is related to an issue (rh-che#668).
In OpenShift, using a
DeploymentConfiginstead of a barePodfor workspaces makes more sense, as loosePods are intended to be used for short-running jobs. As we intend workspaces to run until terminated by a user, aDeploymentConfigmakes more sense in this context.Further, using
DeploymentConfigscould enable us to use some of OpenShift's features for managing running workspaces later.Release Notes
Use Deployments instead of Pods for managing workspaces when using Kubernetes/Openshift infrastructure.
Docs PR
N/A