Skip to content

Commit

Permalink
CHE-9431 Fix removing of PVCs when OpenShift project name is predefined
Browse files Browse the repository at this point in the history
  • Loading branch information
@sleshchenko
sleshchenko committed Apr 16, 2018
1 parent 7d31a5a commit 778d862
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 78 deletions.
8 changes: 8 additions & 0 deletions ...eclipse/che/workspace/infrastructure/kubernetes/namespace/KubernetesNamespaceFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,14 @@ public KubernetesNamespaceFactory(
this.clientFactory = clientFactory;
}

/**
* Returns true if namespace is predefined for all workspaces or false if each workspace will
* be provided with a new namespace.
*/
public boolean isPredefined() {
return isNullOrEmpty(namespaceName);
}

/**
* Creates a Kubernetes namespace for the specified workspace.
*
Expand Down
21 changes: 21 additions & 0 deletions ...e/che/workspace/infrastructure/kubernetes/namespace/KubernetesPersistentVolumeClaims.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import io.fabric8.kubernetes.client.KubernetesClientException;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory;
Expand All @@ -28,6 +29,7 @@
* @author Sergii Leshchenko
*/
public class KubernetesPersistentVolumeClaims {

private final String namespace;
private final String workspaceId;
private final KubernetesClientFactory clientFactory;
Expand Down Expand Up @@ -115,4 +117,23 @@ public void createIfNotExist(Collection<PersistentVolumeClaim> toCreate)
}
}
}

/**
* Removes all PVCs which have the specified labels.
*
* @param labels labels to filter PVCs
* @throws InfrastructureException when any error occurs while removing
*/
public void delete(Map<String, String> labels) throws InfrastructureException {
try {
clientFactory
.create(workspaceId)
.persistentVolumeClaims()
.inNamespace(namespace)
.withLabels(labels)
.delete();
} catch (KubernetesClientException e) {
throw new KubernetesInfrastructureException(e);
}
}
}
17 changes: 4 additions & 13 deletions ...pse/che/workspace/infrastructure/kubernetes/namespace/pvc/UniqueWorkspacePVCStrategy.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesObjectUtil.putLabel;
import static org.eclipse.che.workspace.infrastructure.kubernetes.provision.LogsVolumeMachineProvisioner.LOGS_VOLUME_NAME;

import com.google.common.collect.ImmutableMap;
import io.fabric8.kubernetes.api.model.Container;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.api.model.PersistentVolumeClaim;
Expand All @@ -32,8 +33,6 @@
import org.eclipse.che.api.core.model.workspace.config.Volume;
import org.eclipse.che.api.core.model.workspace.runtime.RuntimeIdentity;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.commons.annotation.Nullable;
import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory;
import org.eclipse.che.workspace.infrastructure.kubernetes.Names;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory;
Expand Down Expand Up @@ -65,25 +64,19 @@ public class UniqueWorkspacePVCStrategy implements WorkspaceVolumesStrategy {
public static final String UNIQUE_STRATEGY = "unique";

private final String pvcNamePrefix;
private final String namespaceName;
private final String pvcQuantity;
private final String pvcAccessMode;
private final KubernetesClientFactory clientFactory;
private final KubernetesNamespaceFactory factory;

@Inject
public UniqueWorkspacePVCStrategy(
@Nullable @Named("che.infra.kubernetes.namespace") String namespaceName,
@Named("che.infra.kubernetes.pvc.name") String pvcNamePrefix,
@Named("che.infra.kubernetes.pvc.quantity") String pvcQuantity,
@Named("che.infra.kubernetes.pvc.access_mode") String pvcAccessMode,
KubernetesNamespaceFactory factory,
KubernetesClientFactory clientFactory) {
KubernetesNamespaceFactory factory) {
this.pvcNamePrefix = pvcNamePrefix;
this.pvcQuantity = pvcQuantity;
this.namespaceName = namespaceName;
this.pvcAccessMode = pvcAccessMode;
this.clientFactory = clientFactory;
this.factory = factory;
}

Expand Down Expand Up @@ -173,12 +166,10 @@ else if (provisionedVolumeName2PVC.containsKey(volumeName)) {

@Override
public void cleanup(String workspaceId) throws InfrastructureException {
clientFactory
factory
.create(workspaceId)
.persistentVolumeClaims()
.inNamespace(namespaceName)
.withLabel(CHE_WORKSPACE_ID_LABEL, workspaceId)
.delete();
.delete(ImmutableMap.of(CHE_WORKSPACE_ID_LABEL, workspaceId));
}

private void addVolumeIfAbsent(PodSpec podSpec, String pvcUniqueName) {
Expand Down
35 changes: 14 additions & 21 deletions ...rg/eclipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleaner.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,13 @@
*/
package org.eclipse.che.workspace.infrastructure.kubernetes.namespace.pvc;

import static com.google.common.base.Strings.isNullOrEmpty;

import com.google.inject.Inject;
import com.google.inject.Singleton;
import javax.inject.Named;
import org.eclipse.che.api.core.notification.EventService;
import org.eclipse.che.api.core.notification.EventSubscriber;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.api.workspace.shared.event.WorkspaceRemovedEvent;
import org.eclipse.che.commons.annotation.Nullable;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -38,36 +35,32 @@ public class WorkspacePVCCleaner {
private static final Logger LOG = LoggerFactory.getLogger(WorkspacePVCCleaner.class);

private final boolean pvcEnabled;
private final String namespaceName;
private final WorkspaceVolumesStrategy strategy;
private final KubernetesNamespaceFactory namespaceFactory;

@Inject
public WorkspacePVCCleaner(
@Named("che.infra.kubernetes.pvc.enabled") boolean pvcEnabled,
@Nullable @Named("che.infra.kubernetes.namespace") String namespaceName,
KubernetesNamespaceFactory namespaceFactory,
WorkspaceVolumesStrategy pvcStrategy) {
this.pvcEnabled = pvcEnabled;
this.namespaceName = namespaceName;
this.namespaceFactory = namespaceFactory;
this.strategy = pvcStrategy;
}

@Inject
public void subscribe(EventService eventService) {
if (pvcEnabled && !isNullOrEmpty(namespaceName))
if (pvcEnabled && !namespaceFactory.isPredefined())
eventService.subscribe(
new EventSubscriber<WorkspaceRemovedEvent>() {
@Override
public void onEvent(WorkspaceRemovedEvent event) {
final String workspaceId = event.getWorkspace().getId();
try {
strategy.cleanup(workspaceId);
} catch (InfrastructureException ex) {
LOG.error(
"Failed to cleanup workspace '{}' data. Cause: {}",
workspaceId,
ex.getMessage());
}
event -> {
final String workspaceId = event.getWorkspace().getId();
try {
strategy.cleanup(workspaceId);
} catch (InfrastructureException ex) {
LOG.error(
"Failed to cleanup workspace '{}' data. Cause: {}", workspaceId, ex.getMessage());
}
});
},
WorkspaceRemovedEvent.class);
}
}
39 changes: 2 additions & 37 deletions ...che/workspace/infrastructure/kubernetes/namespace/pvc/UniqueWorkspacePVCStrategyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import static org.eclipse.che.workspace.infrastructure.kubernetes.Constants.CHE_WORKSPACE_ID_LABEL;
import static org.eclipse.che.workspace.infrastructure.kubernetes.namespace.pvc.CommonPVCStrategyTest.mockName;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
Expand All @@ -34,10 +33,6 @@
import io.fabric8.kubernetes.api.model.PersistentVolumeClaim;
import io.fabric8.kubernetes.api.model.Pod;
import io.fabric8.kubernetes.api.model.PodSpec;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.fabric8.kubernetes.client.dsl.FilterWatchListDeletable;
import io.fabric8.kubernetes.client.dsl.MixedOperation;
import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
Expand All @@ -47,7 +42,6 @@
import org.eclipse.che.api.workspace.server.model.impl.VolumeImpl;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.api.workspace.server.spi.environment.InternalMachineConfig;
import org.eclipse.che.workspace.infrastructure.kubernetes.KubernetesClientFactory;
import org.eclipse.che.workspace.infrastructure.kubernetes.environment.KubernetesEnvironment;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespace;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory;
Expand All @@ -67,7 +61,6 @@
public class UniqueWorkspacePVCStrategyTest {

private static final String WORKSPACE_ID = "workspace123";
private static final String NAMESPACE_NAME = "che";
private static final String PVC_NAME_PREFIX = "che-claim";
private static final String POD_NAME = "main";
private static final String POD_NAME_2 = "second";
Expand All @@ -86,8 +79,6 @@ public class UniqueWorkspacePVCStrategyTest {
new RuntimeIdentityImpl(WORKSPACE_ID, "env1", "id1");

@Mock private KubernetesEnvironment k8sEnv;
@Mock private KubernetesClientFactory clientFactory;
@Mock private KubernetesClient client;
@Mock private KubernetesNamespaceFactory factory;
@Mock private KubernetesNamespace k8sNamespace;
@Mock private KubernetesPersistentVolumeClaims pvcs;
Expand All @@ -104,10 +95,7 @@ public class UniqueWorkspacePVCStrategyTest {
@BeforeMethod
public void setup() throws Exception {
strategy =
new UniqueWorkspacePVCStrategy(
NAMESPACE_NAME, PVC_NAME_PREFIX, PVC_QUANTITY, PVC_ACCESS_MODE, factory, clientFactory);
when(clientFactory.create()).thenReturn(client);
when(clientFactory.create(anyString())).thenReturn(client);
new UniqueWorkspacePVCStrategy(PVC_NAME_PREFIX, PVC_QUANTITY, PVC_ACCESS_MODE, factory);

Map<String, InternalMachineConfig> machines = new HashMap<>();
InternalMachineConfig machine1 = mock(InternalMachineConfig.class);
Expand Down Expand Up @@ -239,32 +227,9 @@ public void throwsInfrastructureExceptionWhenFailedToCreatePVCs() throws Excepti

@Test
public void testRemovesPVCWhenCleanupCalled() throws Exception {
final MixedOperation mixedOperation = mock(MixedOperation.class);
final NonNamespaceOperation namespace = mock(NonNamespaceOperation.class);
final FilterWatchListDeletable filterList = mock(FilterWatchListDeletable.class);
doReturn(mixedOperation).when(client).persistentVolumeClaims();
doReturn(namespace).when(mixedOperation).inNamespace(NAMESPACE_NAME);
doReturn(filterList).when(namespace).withLabel(CHE_WORKSPACE_ID_LABEL, WORKSPACE_ID);
when(filterList.delete()).thenReturn(true);

strategy.cleanup(WORKSPACE_ID);

verify(filterList).delete();
}

@Test
public void testDoNothingWhenNoPVCFoundInNamespaceOnCleanup() throws Exception {
final MixedOperation mixedOperation = mock(MixedOperation.class);
final NonNamespaceOperation namespace = mock(NonNamespaceOperation.class);
final FilterWatchListDeletable filterList = mock(FilterWatchListDeletable.class);
doReturn(mixedOperation).when(client).persistentVolumeClaims();
doReturn(namespace).when(mixedOperation).inNamespace(NAMESPACE_NAME);
doReturn(filterList).when(namespace).withLabel(CHE_WORKSPACE_ID_LABEL, WORKSPACE_ID);
when(filterList.delete()).thenReturn(false);

strategy.cleanup(WORKSPACE_ID);

verify(filterList).delete();
verify(pvcs).delete(ImmutableMap.of(CHE_WORKSPACE_ID_LABEL, WORKSPACE_ID));
}

private static PersistentVolumeClaim mockPVC(Map<String, String> labels, String name) {
Expand Down
27 changes: 20 additions & 7 deletions ...clipse/che/workspace/infrastructure/kubernetes/namespace/pvc/WorkspacePVCCleanerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
package org.eclipse.che.workspace.infrastructure.kubernetes.namespace.pvc;

import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
Expand All @@ -24,6 +25,7 @@
import org.eclipse.che.api.core.notification.EventSubscriber;
import org.eclipse.che.api.workspace.server.spi.InfrastructureException;
import org.eclipse.che.api.workspace.shared.event.WorkspaceRemovedEvent;
import org.eclipse.che.workspace.infrastructure.kubernetes.namespace.KubernetesNamespaceFactory;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.BeforeMethod;
Expand All @@ -39,32 +41,43 @@
public class WorkspacePVCCleanerTest {

private static final String WORKSPACE_ID = "workspace132";
private static final String NAMESPACE_NAME = "che";

@Mock private WorkspaceVolumesStrategy pvcStrategy;
@Mock private EventService eventService;
@Mock private KubernetesNamespaceFactory namespaceFactory;

private WorkspacePVCCleaner workspacePVCCleaner;

@BeforeMethod
public void setUp() {
workspacePVCCleaner = new WorkspacePVCCleaner(true, NAMESPACE_NAME, pvcStrategy);
when(namespaceFactory.isPredefined()).thenReturn(false);
workspacePVCCleaner = new WorkspacePVCCleaner(true, namespaceFactory, pvcStrategy);
}

@Test
public void testDoNotSubscribesCleanerWhenPVCDisabled() {
workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, NAMESPACE_NAME, pvcStrategy));
workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy));

workspacePVCCleaner.subscribe(eventService);

verify(eventService, never()).subscribe(any());
verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class));
}

@Test
public void testDoNotSubscribesCleanerWhenPVCEnabledAndNamespaceIsPredefined() {
when(namespaceFactory.isPredefined()).thenReturn(true);
workspacePVCCleaner = spy(new WorkspacePVCCleaner(false, namespaceFactory, pvcStrategy));

workspacePVCCleaner.subscribe(eventService);

verify(eventService, never()).subscribe(any(), eq(WorkspaceRemovedEvent.class));
}

@Test
public void testSubscribesDeleteKubernetesOnWorkspaceRemove() throws Exception {
workspacePVCCleaner.subscribe(eventService);

verify(eventService).subscribe(any(EventSubscriber.class));
verify(eventService).subscribe(any(), eq(WorkspaceRemovedEvent.class));
}

@Test
Expand All @@ -81,7 +94,7 @@ public void testInvokeCleanupWhenWorkspaceRemovedEventPublished() throws Excepti
return invocationOnMock;
})
.when(eventService)
.subscribe(any());
.subscribe(any(), eq(WorkspaceRemovedEvent.class));

workspacePVCCleaner.subscribe(eventService);

Expand All @@ -102,7 +115,7 @@ public void testDoNotRethrowExceptionWhenErrorOnCleanupOccurs() throws Exception
return invocationOnMock;
})
.when(eventService)
.subscribe(any());
.subscribe(any(), eq(WorkspaceRemovedEvent.class));
doThrow(InfrastructureException.class).when(pvcStrategy).cleanup(WORKSPACE_ID);

workspacePVCCleaner.subscribe(eventService);
Expand Down

0 comments on commit 778d862

Please sign in to comment.