Skip to content

Commit

Permalink
[#4341] Remove deleted flags from server.conf
Browse files Browse the repository at this point in the history 
Summary: Currently we persist flag removal in the YW DB, but we don't delete the entry from the server conf file. This means that on restart, we pick up this entry that should have been deleted. Fix this by adding an ansible task that takes in parameter `gflags_to_remove` list and removes any entries in the server conf that match an entry in this list. This list is generated by YW taking a difference between the UserIntent flags and taskParams() flags.

Test Plan: UpgradeUniverseTest and NodeManagerTest unit tests as well as manual testing of just flag deletion as well as flag deletion + addition together.

Reviewers: bogdan, ram

Reviewed By: ram

Subscribers: jenkins-bot, yugaware

Differential Revision: https://phabricator.dev.yugabyte.com/D8432
  • Loading branch information
@rahuldesirazu
rahuldesirazu committed May 9, 2020
1 parent c362661 commit c4c415e
Show file tree
Hide file tree
Showing 9 changed files with 138 additions and 29 deletions.
4 changes: 4 additions & 0 deletions managed/devops/opscli/ybops/cloud/common/method.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,7 @@ def prepare(self):
self.parser.add_argument('--extra_gflags', default=None)
self.parser.add_argument('--gflags', default=None)
self.parser.add_argument('--replace_gflags', action="store_true")
self.parser.add_argument('--gflags_to_remove', default=None)
self.parser.add_argument('--master_addresses_for_tserver')
self.parser.add_argument('--master_addresses_for_master')
self.parser.add_argument('--rootCA_cert')
Expand Down Expand Up @@ -507,6 +508,9 @@ def callback(self, args):
if args.extra_gflags is not None:
self.extra_vars["extra_gflags"] = json.loads(args.extra_gflags)

if args.gflags_to_remove is not None:
self.extra_vars["gflags_to_remove"] = json.loads(args.gflags_to_remove)

if args.rootCA_cert is not None:
self.extra_vars["rootCA_cert"] = args.rootCA_cert

Expand Down
8 changes: 8 additions & 0 deletions managed/devops/roles/configure-cluster-server/tasks/main.yml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,14 @@
create: yes
with_dict: "{{ gflags | default(_gflags) }}"

- name: Configure | Delete {{ yb_process_type }} gflags
lineinfile:
dest: "{{ yb_conf_file }}"
regexp: "^--{{ item }}=(.*)"
state: absent
with_items: "{{ gflags_to_remove }}"
when: gflags_to_remove is defined

- name: Configure | Add yb server ctl script
vars:
mount_paths: "{{ _mount_points | join(' ') }}"
Expand Down
10 changes: 6 additions & 4 deletions managed/src/main/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,7 @@ public void run() {
didUpgradeUniverse = true;
break;
case GFlags:
if (!taskParams().masterGFlags.isEmpty() &&
!taskParams().masterGFlags.equals(primIntent.masterGFlags)) {
if (!taskParams().masterGFlags.equals(primIntent.masterGFlags)) {
LOG.info("Updating Master gflags: {} for {} nodes in universe {}",
taskParams().masterGFlags, masterNodes.size(), universe.name);
if (!taskParams().rollingUpgrade) {
Expand All @@ -157,8 +156,7 @@ public void run() {
}
didUpgradeUniverse = true;
}
if (!taskParams().tserverGFlags.isEmpty() &&
!taskParams().tserverGFlags.equals(primIntent.tserverGFlags)) {
if (!taskParams().tserverGFlags.equals(primIntent.tserverGFlags)) {
LOG.info("Updating T-Server gflags: {} for {} nodes in universe {}",
taskParams().tserverGFlags, tServerNodes.size(), universe.name);
if (taskParams().rollingUpgrade) {
Expand Down Expand Up @@ -357,8 +355,12 @@ private AnsibleConfigureServers getConfigureTask(NodeDetails node,
} else if (type == UpgradeTaskType.GFlags) {
if (processType.equals(ServerType.MASTER)) {
params.gflags = taskParams().masterGFlags;
params.gflagsToRemove = userIntent.masterGFlags.keySet().stream().filter(
flag -> !taskParams().masterGFlags.containsKey(flag)).collect(Collectors.toSet());
} else {
params.gflags = taskParams().tserverGFlags;
params.gflagsToRemove = userIntent.tserverGFlags.keySet().stream().filter(
flag -> !taskParams().tserverGFlags.containsKey(flag)).collect(Collectors.toSet());
}
}

Expand Down
3 changes: 3 additions & 0 deletions ...ed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/AnsibleConfigureServers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@

import java.util.HashMap;
import java.util.Map;
import java.util.HashSet;
import java.util.Set;

public class AnsibleConfigureServers extends NodeTaskBase {
public static final Logger LOG = LoggerFactory.getLogger(AnsibleConfigureServers.class);
Expand All @@ -37,6 +39,7 @@ public static class Params extends NodeTaskParams {
public boolean enableClientToNodeEncrypt = false;
public boolean allowInsecure = true;
public Map<String, String> gflags = new HashMap<>();
public Set<String> gflagsToRemove = new HashSet<>();
public boolean updateMasterAddrsOnly = false;
public CollectionLevel callhomeLevel;
// Development params.
Expand Down
13 changes: 10 additions & 3 deletions managed/src/main/java/com/yugabyte/yw/common/NodeManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,10 +305,12 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
case GFlags:
{
if (!taskParam.updateMasterAddrsOnly &&
(taskParam.gflags == null || taskParam.gflags.isEmpty())) {
throw new RuntimeException(taskParam.gflags + " GFlags data provided for " +
(taskParam.gflags == null || taskParam.gflags.isEmpty()) &&
(taskParam.gflagsToRemove == null || taskParam.gflagsToRemove.isEmpty())) {
throw new RuntimeException("GFlags data provided for " +
taskParam.nodeName + "'s " +
taskParam.getProperty("processType") + " process.");
taskParam.getProperty("processType") + " process" +
" have no changes from existing flags.");
}

String processType = taskParam.getProperty("processType");
Expand Down Expand Up @@ -336,6 +338,11 @@ private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskP
}
subcommand.add("--gflags");
subcommand.add(Json.stringify(Json.toJson(gflags)));

if (taskParam.gflagsToRemove != null && !taskParam.gflagsToRemove.isEmpty()) {
subcommand.add("--gflags_to_remove");
subcommand.add(Json.stringify(Json.toJson(taskParam.gflagsToRemove)));
}
}
break;
}
Expand Down
3 changes: 1 addition & 2 deletions managed/src/main/java/com/yugabyte/yw/controllers/UniverseController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1258,8 +1258,7 @@ public Result upgrade(UUID customerUUID, UUID universeUUID) {
break;
case GFlags:
customerTaskType = CustomerTask.TaskType.UpgradeGflags;
if ((taskParams.masterGFlags == null || taskParams.masterGFlags.isEmpty()) &&
(taskParams.tserverGFlags == null || taskParams.tserverGFlags.isEmpty())) {
if (taskParams.masterGFlags == null && taskParams.tserverGFlags == null) {
return ApiResponse.error(
BAD_REQUEST,
"gflags param is required for taskType: " + taskParams.taskType);
Expand Down
74 changes: 66 additions & 8 deletions managed/src/test/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,13 +228,21 @@ private int assertSoftwareUpgradeSequence(Map<Integer, List<TaskInfo>> subTasksB
private int assertGFlagsUpgradeSequence(Map<Integer, List<TaskInfo>> subTasksByPosition,
ServerType serverType, int startPosition, boolean isRollingUpgrade) {
return assertGFlagsUpgradeSequence(subTasksByPosition, serverType, startPosition,
isRollingUpgrade, false);
isRollingUpgrade, false, false);
}

private int assertGFlagsUpgradeSequence(Map<Integer, List<TaskInfo>> subTasksByPosition,
ServerType serverType, int startPosition,
boolean isRollingUpgrade, boolean isEdit) {
return assertGFlagsUpgradeSequence(subTasksByPosition, serverType, startPosition,
isRollingUpgrade, isEdit, false);
}

private int assertGFlagsUpgradeSequence(Map<Integer, List<TaskInfo>> subTasksByPosition,
ServerType serverType,
int startPosition, boolean isRollingUpgrade,
boolean isEdit) {
boolean isEdit,
boolean isDelete) {
int position = startPosition;
if (isRollingUpgrade) {
List<TaskType> taskSequence = GFLAGS_ROLLING_UPGRADE_TASK_SEQUENCE;
Expand All @@ -252,12 +260,13 @@ private int assertGFlagsUpgradeSequence(Map<Integer, List<TaskInfo>> subTasksByP
));

if (taskType.equals(TaskType.AnsibleConfigureServers)) {
JsonNode gflagValue = serverType.equals(MASTER) ?
Json.parse("{\"master-flag\":" + (isEdit ? "\"m2\"}" : "\"m1\"}")) :
Json.parse("{\"tserver-flag\":" + (isEdit ? "\"t2\"}" : "\"t1\"}"));
assertValues.putAll(ImmutableMap.of(
"gflags", gflagValue, "processType", serverType.toString()
));
if (!isDelete) {
JsonNode gflagValue = serverType.equals(MASTER) ?
Json.parse("{\"master-flag\":" + (isEdit ? "\"m2\"}" : "\"m1\"}")) :
Json.parse("{\"tserver-flag\":" + (isEdit ? "\"t2\"}" : "\"t1\"}"));
assertValues.putAll(ImmutableMap.of("gflags", gflagValue));
}
assertValues.put("processType", serverType.toString());
}
assertNodeSubTask(tasks, assertValues);
}
Expand Down Expand Up @@ -661,4 +670,53 @@ public void run(Universe universe) {
UpgradeType.ROLLING_UPGRADE_MASTER_ONLY, true);
assertEquals(26, position);
}

@Test
public void testRemoveFlags() {
for (ServerType serverType : ImmutableList.of(MASTER, TSERVER)) {
// Simulate universe created with master flags and tserver flags.
final Map<String, String> tserverFlags = ImmutableMap.of("tserver-flag", "m123");
final Map<String, String> masterGFlags = ImmutableMap.of("master-flag", "m123");
Universe.UniverseUpdater updater = new Universe.UniverseUpdater() {
public void run(Universe universe) {
UniverseDefinitionTaskParams universeDetails = universe.getUniverseDetails();
UserIntent userIntent = universeDetails.getPrimaryCluster().userIntent;
userIntent.masterGFlags = masterGFlags;
userIntent.tserverGFlags = tserverFlags;
universe.setUniverseDetails(universeDetails);
}
};
Universe.saveDetails(defaultUniverse.universeUUID, updater);

UpgradeUniverse.Params taskParams = new UpgradeUniverse.Params();
// This is a delete operation on the master flags.
if (serverType == MASTER) {
taskParams.masterGFlags = new HashMap<>();
taskParams.tserverGFlags = tserverFlags;
} else {
taskParams.masterGFlags = masterGFlags;
taskParams.tserverGFlags = new HashMap<>();
}

int expectedVersion = serverType == MASTER ? 3 : 14;
TaskInfo taskInfo = submitTask(taskParams,
UpgradeUniverse.UpgradeTaskType.GFlags,
expectedVersion);

int numInvocations = serverType == MASTER ? 9 : 18;
verify(mockNodeManager, times(numInvocations)).nodeCommand(any(), any());

List<TaskInfo> subTasks = new ArrayList<>(taskInfo.getSubTasks());
Map<Integer, List<TaskInfo>> subTasksByPosition =
subTasks.stream().collect(Collectors.groupingBy(w -> w.getPosition()));
int position = serverType == MASTER ? 0 : 1;
position = assertGFlagsUpgradeSequence(subTasksByPosition, serverType, position,
true, true, true);
position = assertGFlagsCommonTasks(subTasksByPosition, position, serverType == MASTER ?
UpgradeType.ROLLING_UPGRADE_MASTER_ONLY : UpgradeType.ROLLING_UPGRADE_TSERVER_ONLY,
true);
assertEquals(serverType == MASTER ? 26 : 28, position);
}

}
}
44 changes: 36 additions & 8 deletions managed/src/test/java/com/yugabyte/yw/common/NodeManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import java.util.Map;
import java.util.UUID;
import java.util.function.Predicate;
import java.util.Set;
import java.util.HashSet;

import static com.yugabyte.yw.commissioner.tasks.UniverseDefinitionTaskBase.ServerType.MASTER;
import static com.yugabyte.yw.commissioner.tasks.UniverseDefinitionTaskBase.ServerType.TSERVER;
Expand Down Expand Up @@ -356,14 +358,16 @@ private List<String> nodeCommand(
expectedCommand.add("--extra_gflags");
expectedCommand.add(Json.stringify(Json.toJson(gflags)));
} else if (configureParams.type == GFlags) {
if (!configureParams.gflags.isEmpty()) {
String processType = configureParams.getProperty("processType");
expectedCommand.add("--yb_process_type");
expectedCommand.add(processType.toLowerCase());
String gflagsJson = Json.stringify(Json.toJson(gflags));
expectedCommand.add("--replace_gflags");
expectedCommand.add("--gflags");
expectedCommand.add(gflagsJson);
String processType = configureParams.getProperty("processType");
expectedCommand.add("--yb_process_type");
expectedCommand.add(processType.toLowerCase());
String gflagsJson = Json.stringify(Json.toJson(gflags));
expectedCommand.add("--replace_gflags");
expectedCommand.add("--gflags");
expectedCommand.add(gflagsJson);
if (configureParams.gflagsToRemove != null && !configureParams.gflagsToRemove.isEmpty()) {
expectedCommand.add("--gflags_to_remove");
expectedCommand.add(Json.stringify(Json.toJson(configureParams.gflagsToRemove)));
}
} else {
expectedCommand.add("--extra_gflags");
Expand Down Expand Up @@ -881,6 +885,30 @@ public void testSoftwareUpgradeWithoutReleasePackage() {
}
}

@Test
public void testGFlagRemove() {
for (TestData t : testData) {
for (String serverType : ImmutableList.of(MASTER.toString(), TSERVER.toString())) {
AnsibleConfigureServers.Params params = new AnsibleConfigureServers.Params();
buildValidParams(t, params, Universe.saveDetails(createUniverse().universeUUID,
ApiUtils.mockUniverseUpdater(t.cloudType)));
Set<String> gflagsToRemove = new HashSet<>();
gflagsToRemove.add("flag1");
gflagsToRemove.add("flag2");
params.type = GFlags;
params.isMasterInShellMode = true;
params.gflagsToRemove = gflagsToRemove;
params.setProperty("processType", serverType);

List<String> expectedCommand = new ArrayList<>(t.baseCommand);
expectedCommand.addAll(nodeCommand(NodeManager.NodeCommandType.Configure, params, t));
nodeManager.nodeCommand(NodeManager.NodeCommandType.Configure, params);
verify(shellProcessHandler, times(1)).run(expectedCommand,
t.region.provider.getConfig());
}
}
}

@Test
public void testGFlagsUpgradeNullProcessType() {
for (TestData t : testData) {
Expand Down
8 changes: 4 additions & 4 deletions managed/src/test/java/com/yugabyte/yw/controllers/UniverseControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,7 +1021,7 @@ public void testUniverseGFlagsUpgradeWithInvalidParams() {
String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade";
Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson);

assertBadRequest(result, "gflags param is required for taskType: GFlags");
assertBadRequest(result, "Neither master nor tserver gflags changed.");
assertAuditEntry(0, customer.uuid);
}

Expand Down Expand Up @@ -1081,7 +1081,7 @@ public void testUniverseGFlagsUpgradeWithMissingGflags() {
String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade";
Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJsonMissingGFlags);

assertBadRequest(result, "gflags param is required for taskType: GFlags");
assertBadRequest(result, "Neither master nor tserver gflags changed.");
assertAuditEntry(0, customer.uuid);
}

Expand All @@ -1104,7 +1104,7 @@ public void testUniverseGFlagsUpgradeWithMalformedTServerFlags() {
String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade";
Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson);

assertBadRequest(result, "gflags param is required for taskType: GFlags");
assertBadRequest(result, "Neither master nor tserver gflags changed.");
assertAuditEntry(0, customer.uuid);
}

Expand All @@ -1127,7 +1127,7 @@ public void testUniverseGFlagsUpgradeWithMalformedMasterGFlags() {
String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade";
Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson);

assertBadRequest(result, "gflags param is required for taskType: GFlags");
assertBadRequest(result, "Neither master nor tserver gflags changed.");
assertAuditEntry(0, customer.uuid);
}

Expand Down

0 comments on commit c4c415e

Please sign in to comment.