Skip to content
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

HDDS-8489. Refactor GET request to POST for OM DB snapshot #4695

Merged
merged 15 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ public final class OzoneConsts {
public static final String RANGER_OZONE_SERVICE_VERSION_KEY =
"#RANGEROZONESERVICEVERSION";

public static final String MULTIPART_FORM_DATA_BOUNDARY = "---XXX";

/**
* Supports Bucket Versioning.
*/
Expand Down
5 changes: 5 additions & 0 deletions hadoop-hdds/framework/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ https://maven.apache.org/xsd/maven-4.0.0.xsd">
<groupId>org.apache.commons</groupId>
<artifactId>commons-configuration2</artifactId>
</dependency>
<dependency>
<groupId>commons-fileupload</groupId>
<artifactId>commons-fileupload</artifactId>
<version>${commons-fileupload.version}</version>
</dependency>
<dependency>
<groupId>org.apache.logging.log4j</groupId>
<artifactId>log4j-core</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
import java.util.Objects;
import java.util.stream.Collectors;

import org.apache.commons.fileupload.FileItemIterator;
import org.apache.commons.fileupload.FileItemStream;
import org.apache.commons.fileupload.servlet.ServletFileUpload;
import org.apache.commons.fileupload.util.Streams;
import org.apache.hadoop.hdds.server.OzoneAdmins;
import org.apache.hadoop.hdds.utils.db.DBCheckpoint;
import org.apache.hadoop.hdds.utils.db.DBStore;
Expand Down Expand Up @@ -94,15 +98,13 @@ private boolean hasPermission(UserGroupInformation user) {
}

/**
* Process a GET request for the DB checkpoint snapshot.
*
* @param request The servlet request we are processing
* @param response The servlet response we are creating
* Generates Snapshot checkpoint as tar ball.
* @param request the HTTP servlet request
* @param response the HTTP servlet response
* @param isFormData indicator whether request is form data
*/
@Override
public void doGet(HttpServletRequest request, HttpServletResponse response) {

LOG.info("Received request to obtain DB checkpoint snapshot");
private void generateSnapshotCheckpoint(HttpServletRequest request,
HttpServletResponse response, boolean isFormData) {
if (dbStore == null) {
LOG.error(
"Unable to process metadata snapshot request. DB Store is null");
Expand Down Expand Up @@ -151,7 +153,8 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {

List<String> receivedSstList = new ArrayList<>();
List<String> excludedSstList = new ArrayList<>();
String[] sstParam = request.getParameterValues(
String[] sstParam = isFormData ?
parseFormDataParameters(request) : request.getParameterValues(
OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST);
if (sstParam != null) {
receivedSstList.addAll(
Expand Down Expand Up @@ -216,6 +219,68 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) {
}
}

/**
* Parses request form data parameters.
* @param request the HTTP servlet request
* @return array of parsed sst form data parameters for exclusion
*/
private static String[] parseFormDataParameters(HttpServletRequest request) {
String fieldName = OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIELD_NAME constant is created.

ServletFileUpload upload = new ServletFileUpload();
List<String> sstParam = new ArrayList<>();

try {
FileItemIterator iter = upload.getItemIterator(request);
while (iter.hasNext()) {
FileItemStream item = iter.next();
if (!item.isFormField()) {
continue;
}

if (!item.getFieldName().equals(fieldName)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if conditions can be combined to one.

Suggested change
if (!item.getFieldName().equals(fieldName)) {
if (!fieldName.equals(item.getFieldName())

to avoid NPE because we know fieldName is constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditions are combined and comparation is reversed.

continue;
}

sstParam.add(Streams.asString(item.openStream()));
}
} catch (Exception e) {
LOG.warn("Exception occured during form data parsing {}", e.getMessage());
}

return sstParam.size() == 0 ? null : sstParam.toArray(new String[0]);
}

/**
* Process a GET request for the DB checkpoint snapshot.
*
* @param request The servlet request we are processing
* @param response The servlet response we are creating
*/
@Override
public void doGet(HttpServletRequest request, HttpServletResponse response) {
LOG.info("Received GET request to obtain DB checkpoint snapshot");

generateSnapshotCheckpoint(request, response, false);
}

/**
* Process a POST request for the DB checkpoint snapshot.
*
* @param request The servlet request we are processing
* @param response The servlet response we are creating
*/
@Override
public void doPost(HttpServletRequest request, HttpServletResponse response) {
LOG.info("Received POST request to obtain DB checkpoint snapshot");

if (!ServletFileUpload.isMultipartContent(request)) {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
return;
}

generateSnapshotCheckpoint(request, response, true);
}

/**
* Write checkpoint to the stream.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@
import java.net.MalformedURLException;
import java.net.URISyntaxException;
import java.net.URL;
import java.util.List;

import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_REQUEST_FLUSH;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_HTTP_ENDPOINT;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_ADDRESS_KEY;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_RATIS_PORT_DEFAULT;
import static org.apache.hadoop.ozone.om.OMConfigKeys.OZONE_OM_RATIS_PORT_KEY;
Expand Down Expand Up @@ -164,8 +162,8 @@ public OMNodeDetails build() {
}
}

public URL getOMDBCheckpointEndpointUrl(boolean isHttp, boolean flush,
List<String> sstList) throws IOException {
public URL getOMDBCheckpointEndpointUrl(boolean isHttp, boolean flush)
throws IOException {
URL url;
try {
URIBuilder urlBuilder = new URIBuilder().
Expand All @@ -175,12 +173,7 @@ public URL getOMDBCheckpointEndpointUrl(boolean isHttp, boolean flush,
addParameter(OZONE_DB_CHECKPOINT_INCLUDE_SNAPSHOT_DATA, "true").
addParameter(OZONE_DB_CHECKPOINT_REQUEST_FLUSH,
flush ? "true" : "false");
if (sstList != null && !sstList.isEmpty()) {
for (String s: sstList) {
urlBuilder.addParameter(
OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST, s);
}
}

url = urlBuilder.build().toURL();
} catch (URISyntaxException | MalformedURLException e) {
throw new IOException("Could not get OM DB Checkpoint Endpoint Url", e);
Expand Down
1 change: 1 addition & 0 deletions hadoop-ozone/dist/src/main/license/bin/LICENSE.txt
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,7 @@ Apache License 2.0
commons-logging:commons-logging
commons-net:commons-net
commons-validator:commons-validator
commons-fileupload:commons-fileupload
info.picocli:picocli
io.dropwizard.metrics:metrics-core
io.grpc:grpc-api
Expand Down
1 change: 1 addition & 0 deletions hadoop-ozone/dist/src/main/license/jar-report.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ share/ozone/lib/commons-net.jar
share/ozone/lib/commons-pool2.jar
share/ozone/lib/commons-text.jar
share/ozone/lib/commons-validator.jar
share/ozone/lib/commons-fileupload.jar
share/ozone/lib/curator-client.jar
share/ozone/lib/curator-framework.jar
share/ozone/lib/derby.jar
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,20 @@

import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletInputStream;
import javax.servlet.ServletOutputStream;
import javax.servlet.WriteListener;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.UUID;

import org.apache.commons.compress.compressors.CompressorException;
Expand All @@ -39,6 +45,7 @@

import org.apache.commons.io.FileUtils;
import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
import static org.apache.hadoop.ozone.OzoneConsts.MULTIPART_FORM_DATA_BOUNDARY;
import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_REQUEST_FLUSH;

import org.junit.jupiter.api.AfterEach;
Expand All @@ -47,8 +54,12 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.mockito.Matchers;
import org.mockito.Mockito;

import static org.apache.hadoop.ozone.OzoneConsts.OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.doCallRealMethod;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -102,7 +113,7 @@ public void shutdown() {
}

@Test
public void testDoGet()
public void testDoPost()
throws ServletException, IOException, CompressorException,
InterruptedException {

Expand All @@ -129,15 +140,39 @@ public void testDoGet()
when(scmDbCheckpointServletMock.getServletContext())
.thenReturn(servletContextMock);

when(requestMock.getMethod()).thenReturn("POST");
when(requestMock.getContentType()).thenReturn("multipart/form-data; " +
"boundary=" + MULTIPART_FORM_DATA_BOUNDARY);
when(servletContextMock.getAttribute(OzoneConsts.SCM_CONTEXT_ATTRIBUTE))
.thenReturn(cluster.getStorageContainerManager());
when(requestMock.getParameter(OZONE_DB_CHECKPOINT_REQUEST_FLUSH))
.thenReturn("true");

String crNl = "\r\n";
String sstFileName = "sstFile.sst";
byte[] data = ("--" + MULTIPART_FORM_DATA_BOUNDARY + crNl +
"Content-Disposition: form-data; name=\"" +
OZONE_DB_CHECKPOINT_REQUEST_TO_EXCLUDE_SST + "[]\"" + crNl +
crNl +
sstFileName + crNl +
"--" + MULTIPART_FORM_DATA_BOUNDARY + "--" + crNl).getBytes(
StandardCharsets.UTF_8);
InputStream input = new ByteArrayInputStream(data);
ServletInputStream inputStream = Mockito.mock(ServletInputStream.class);
when(requestMock.getInputStream()).thenReturn(inputStream);
when(inputStream.read(any(byte[].class), anyInt(), anyInt()))
.thenAnswer(invocation -> {
byte[] buffer = invocation.getArgument(0);
int offset = invocation.getArgument(1);
int length = invocation.getArgument(2);
return input.read(buffer, offset, length);
});

doNothing().when(responseMock).setContentType("application/x-tgz");
doNothing().when(responseMock).setHeader(Matchers.anyString(),
Matchers.anyString());

tempFile = File.createTempFile("testDoGet_" + System
tempFile = File.createTempFile("testDoPost_" + System
.currentTimeMillis(), ".tar");

FileOutputStream fileOutputStream = new FileOutputStream(tempFile);
Expand All @@ -158,14 +193,14 @@ public void write(int b) throws IOException {
}
});

doCallRealMethod().when(scmDbCheckpointServletMock).doGet(requestMock,
doCallRealMethod().when(scmDbCheckpointServletMock).doPost(requestMock,
responseMock);

scmDbCheckpointServletMock.init();
long initialCheckpointCount =
scmMetrics.getDBCheckpointMetrics().getNumCheckpoints();

scmDbCheckpointServletMock.doGet(requestMock, responseMock);
scmDbCheckpointServletMock.doPost(requestMock, responseMock);

Assertions.assertTrue(tempFile.length() > 0);
Assertions.assertTrue(
Expand All @@ -176,9 +211,51 @@ public void write(int b) throws IOException {
getLastCheckpointStreamingTimeTaken() > 0);
Assertions.assertTrue(scmMetrics.getDBCheckpointMetrics().
getNumCheckpoints() > initialCheckpointCount);

List<String> toExcludeList = new ArrayList<>();
toExcludeList.add(sstFileName);
Mockito.verify(scmDbCheckpointServletMock).writeDbDataToStream(any(),
any(), any(), eq(toExcludeList), any());
} finally {
FileUtils.deleteQuietly(tempFile);
}

}

@Test
public void testDoPostWithInvalidContentType()
throws ServletException, IOException, InterruptedException {

SCMDBCheckpointServlet scmDbCheckpointServletMock =
mock(SCMDBCheckpointServlet.class);

doCallRealMethod().when(scmDbCheckpointServletMock).init();
doCallRealMethod().when(scmDbCheckpointServletMock).initialize(
scm.getScmMetadataStore().getStore(),
scmMetrics.getDBCheckpointMetrics(),
false,
Collections.emptyList(),
Collections.emptyList(),
false);
doCallRealMethod().when(scmDbCheckpointServletMock)
.writeDbDataToStream(any(), any(), any(), any(), any());

HttpServletRequest requestMock = mock(HttpServletRequest.class);
HttpServletResponse responseMock = mock(HttpServletResponse.class);

ServletContext servletContextMock = mock(ServletContext.class);
when(scmDbCheckpointServletMock.getServletContext())
.thenReturn(servletContextMock);

when(requestMock.getContentType()).thenReturn("application/json");

doCallRealMethod().when(scmDbCheckpointServletMock).doPost(requestMock,
responseMock);

scmDbCheckpointServletMock.init();

scmDbCheckpointServletMock.doPost(requestMock, responseMock);

Mockito.verify(responseMock).setStatus(HttpServletResponse.SC_BAD_REQUEST);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
/**
* Package info tests.
*/
package org.apache.hadoop.hdds.scm;