Skip to content

Commit

Permalink
[PLAT-14627][PLAT-14729][PLAT-14726]Process group-based specific gfla…
Browse files Browse the repository at this point in the history 
…gs comprehensively across multiple callsites in YBA

Summary:
Add the gflags belonging to the group in the `getGFlagsForAZ` function to trigger node restarts on GFlag upgrades.
If the user provides values belonging to the group in the user intent, the values are overridden by the group values and user intent is untouched.

Test Plan:
1. Create universe with `ENHANCED_POSTGRES_COMPATIBILTY` group added and verify that the `tserver/conf/server.conf` have correct values of the flag
2. Run a GFlag Upgrade on the above universe to verify user gflags are added to the list
3. Run a GFlag Upgrade to remove the gflag group from the list and verify if all flags belonging to the group are removed.
4. Create a universe with user gflags and gflag group added and verify both of them are correctly merged in the server configuration file.
5. Add a node to the above universe to verify new nodes have the correct gflags.
6. Create a universe with `perAZ` overrides and gflag groups and ensure the user gflags from `perAZ` and the gflag group values correctly override the `perProcessFlags` block
7. Create a universe with groups disabled and run a GFlags upgrade to ensure gflags are correctly applied.
8. Add group gflag as user gflags with different values and verify that the group values are added if group is enabled. Make sure User intent is the same

Reviewers: #yba-api-review!, sneelakantan, vbansal, yshchetinin

Reviewed By: yshchetinin

Subscribers: nbhatia, yugaware

Differential Revision: https://phorge.dev.yugabyte.com/D36623
  • Loading branch information
@Deepti-yb
Deepti-yb committed Jul 24, 2024
1 parent 5ed864d commit e37d791
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 189 deletions.
13 changes: 8 additions & 5 deletions ...ged/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/UpdateAndPersistGFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,14 +98,17 @@ public void run(Universe universe) {
// Updating old maps accordingly
cluster.userIntent.masterGFlags = masterGFlags;
cluster.userIntent.tserverGFlags = tserverGFlags;
if (cluster.userIntent.specificGFlags != null
&& cluster.userIntent.specificGFlags.isInheritFromPrimary()) {
if (cluster.userIntent.specificGFlags != null) {
SpecificGFlags primaryGFlags =
universeDetails.getPrimaryCluster().userIntent.specificGFlags;
if (primaryGFlags != null) {
cluster.userIntent.specificGFlags.setPerProcessFlags(
primaryGFlags.getPerProcessFlags());
cluster.userIntent.specificGFlags.setPerAZ(primaryGFlags.getPerAZ());
if (cluster.userIntent.specificGFlags.isInheritFromPrimary()) {
cluster.userIntent.specificGFlags.setPerProcessFlags(
primaryGFlags.getPerProcessFlags());
cluster.userIntent.specificGFlags.setPerAZ(primaryGFlags.getPerAZ());
}
cluster.userIntent.specificGFlags.setGflagGroups(
primaryGFlags.getGflagGroups());
}
}
}
Expand Down
2 changes: 0 additions & 2 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 @@ -870,8 +870,6 @@ private void processGFlags(
allowOverrideAll,
confGetter,
taskParam);
GFlagsUtil.processGFlagGroups(
gflags, getUserIntentFromParams(taskParam), taskParam.getProperty("processType"));
}

private List<String> getConfigureSubCommand(AnsibleConfigureServers.Params taskParam) {
Expand Down
201 changes: 39 additions & 162 deletions managed/src/main/java/com/yugabyte/yw/common/gflags/GFlagsUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -372,20 +372,12 @@ public static Map<String, String> getAllDefaultGFlags(
return extra_gflags;
}

private static SpecificGFlags.PerProcessFlags processGFlagGroups(
SpecificGFlags specificGFlags, String ybdbVersion) {
// check if any groups are set in specificGFlags
if (specificGFlags == null) {
return null;
}
if (ybdbVersion == null) {
return null;
}
SpecificGFlags result = specificGFlags;
List<GroupName> requestedGroups = specificGFlags.getGflagGroups();
if (requestedGroups.isEmpty()) {
private static List<GFlagGroup> extractGroups(
List<GroupName> requestedGroups, String ybdbVersion) {
if (ybdbVersion == null || requestedGroups == null || requestedGroups.isEmpty()) {
return null;
}
List<GFlagGroup> result = new ArrayList<>();
if (Util.compareYBVersions(
ybdbVersion, GFLAG_GROUPS_STABLE_VERSION, GFLAG_GROUPS_PREVIEW_VERSION, true)
< 0) {
Expand All @@ -411,57 +403,15 @@ private static SpecificGFlags.PerProcessFlags processGFlagGroups(
if (!gFlagGroup.isPresent()) {
throw new PlatformServiceException(BAD_REQUEST, "Unknown gflag group: " + requestedGroup);
}
result = addGFlagGroup(result, gFlagGroup.get());
result.add(gFlagGroup.get());
}
return result;
} catch (IOException e) {
throw new PlatformServiceException(
INTERNAL_SERVER_ERROR,
String.format(
"Failed to fetch GFlag groups for YBDB version %s: %s", ybdbVersion, e.getMessage()));
}

return result.getPerProcessFlags();
}

public static SpecificGFlags addGFlagGroup(SpecificGFlags specificGFlags, GFlagGroup gFlagGroup) {
SpecificGFlags result = specificGFlags;
LOG.info("Adding GFlagGroup {} to SpecificGFlags", gFlagGroup.groupName);
GFlagGroup.ServerTypeFlags flags = gFlagGroup.flags;
if (flags.masterGFlags != null && !flags.masterGFlags.isEmpty()) {
// NOTE: if the same flag is used by multiple groups (like ysql_pg_conf_csv) check
// if existing values are not overridden
Map<String, String> existingMasterGFlags =
result.getPerProcessFlags().value.get(ServerType.MASTER);
if (existingMasterGFlags == null) {
existingMasterGFlags = new HashMap<>();
}
existingMasterGFlags.putAll(flags.masterGFlags);
result.getPerProcessFlags().value.put(ServerType.MASTER, existingMasterGFlags);
}
if (flags.tserverGFlags != null && !flags.tserverGFlags.isEmpty()) {
Map<String, String> existingTserverGFlags =
result.getPerProcessFlags().value.get(ServerType.TSERVER);
String requestedGroupYsqlPgConfCsv = flags.tserverGFlags.remove(YSQL_PG_CONF_CSV);
if (existingTserverGFlags == null) {
existingTserverGFlags = new HashMap<>();
}
String existingTserverYsqlPgConfCsv = existingTserverGFlags.get(YSQL_PG_CONF_CSV);
existingTserverGFlags.putAll(flags.tserverGFlags);
if (existingTserverYsqlPgConfCsv != null && requestedGroupYsqlPgConfCsv != null) {
String ysqlPgConfCsv =
mergeCSVs(requestedGroupYsqlPgConfCsv, existingTserverYsqlPgConfCsv, true);
existingTserverGFlags.put(YSQL_PG_CONF_CSV, ysqlPgConfCsv);
flags.tserverGFlags.put(YSQL_PG_CONF_CSV, requestedGroupYsqlPgConfCsv);
} else {
if (requestedGroupYsqlPgConfCsv != null && !requestedGroupYsqlPgConfCsv.isEmpty()) {
existingTserverGFlags.put(YSQL_PG_CONF_CSV, requestedGroupYsqlPgConfCsv);
flags.tserverGFlags.put(YSQL_PG_CONF_CSV, requestedGroupYsqlPgConfCsv);
}
}
result.getPerProcessFlags().value.put(ServerType.TSERVER, existingTserverGFlags);
}
LOG.info("Added GFlag Group {} to SpecificGFlags", gFlagGroup.groupName);
return result;
}

/** Return the map of ybc flags which will be passed to the db nodes. */
Expand Down Expand Up @@ -1131,35 +1081,40 @@ public static void processUserGFlags(
}

public static void processGFlagGroups(
Map<String, String> userGFlags, UserIntent userIntent, String processType) {
Map<String, String> userGFlags,
UserIntent userIntent,
UniverseTaskBase.ServerType processType) {
if (processType == null) {
return;
}
SpecificGFlags.PerProcessFlags gflagGroupsPerProcess =
processGFlagGroups(userIntent.specificGFlags, userIntent.ybSoftwareVersion);
if (gflagGroupsPerProcess == null) {
if (userIntent.specificGFlags == null) {
return;
}
if (processType.equals(UniverseTaskBase.ServerType.TSERVER.name())) {
Map<String, String> groupTserverGFlags = gflagGroupsPerProcess.value.get(ServerType.TSERVER);
if (groupTserverGFlags != null) {
String groupYsqlPgConfCsv = groupTserverGFlags.remove(YSQL_PG_CONF_CSV);
String userYsqlPgConfCsv = userGFlags.remove(YSQL_PG_CONF_CSV);
userGFlags.putAll(groupTserverGFlags);
if (userYsqlPgConfCsv != null && groupYsqlPgConfCsv != null) {
String ysqlPgConfCsv = mergeCSVs(groupYsqlPgConfCsv, userYsqlPgConfCsv, true);
userGFlags.put(YSQL_PG_CONF_CSV, ysqlPgConfCsv);
} else {
if (groupYsqlPgConfCsv != null && !groupYsqlPgConfCsv.isEmpty()) {
userGFlags.put(YSQL_PG_CONF_CSV, groupYsqlPgConfCsv);
}
}
List<GFlagGroup> gFlagGroups =
extractGroups(userIntent.specificGFlags.getGflagGroups(), userIntent.ybSoftwareVersion);
if (gFlagGroups == null) {
return;
}
for (GFlagGroup gFlagGroup : gFlagGroups) {
GFlagGroup.ServerTypeFlags flags = gFlagGroup.flags;
if (flags == null) {
continue;
}
} else {
Map<String, String> groupMasterGFlags = gflagGroupsPerProcess.value.get(ServerType.MASTER);
if (groupMasterGFlags != null) {
userGFlags.putAll(groupMasterGFlags);
Map<String, String> groupGFlags =
processType == ServerType.MASTER ? flags.masterGFlags : flags.tserverGFlags;
if (groupGFlags == null) {
continue;
}
groupGFlags.forEach(
(k, v) -> {
// If user gflags contains that flag we need to merge.
if (k.equals(YSQL_PG_CONF_CSV) && userGFlags.containsKey(k)) {
// Merging while taking precendence for group
userGFlags.put(k, mergeCSVs(v, userGFlags.get(k), true));
} else {
userGFlags.put(k, v);
}
});
}
}

Expand Down Expand Up @@ -1244,7 +1199,12 @@ public static Map<String, String> getGFlagsForAZ(
}
return getGFlagsForAZ(azUuid, serverType, primary, allClusters);
}
return userIntent.specificGFlags.getGFlags(azUuid, serverType);
Map<String, String> azGFlags = userIntent.specificGFlags.getGFlags(azUuid, serverType);
if (primary.userIntent.specificGFlags != null
&& primary.userIntent.specificGFlags.getGflagGroups() != null) {
processGFlagGroups(azGFlags, primary.userIntent, serverType);
}
return azGFlags;
} else {
if (cluster.clusterType == UniverseDefinitionTaskParams.ClusterType.ASYNC) {
return getGFlagsForAZ(azUuid, serverType, primary, allClusters);
Expand Down Expand Up @@ -1362,29 +1322,6 @@ public static String mergeCSVs(String csv1, String csv2, boolean mergeKeyValues)
return writer.toString();
}

public static String removeCSVs(String csv1, String csv2) {
StringWriter writer = new StringWriter();
try {
CSVFormat csvFormat = CSVFormat.DEFAULT.builder().setRecordSeparator("").build();
try (CSVPrinter csvPrinter = new CSVPrinter(writer, csvFormat)) {
Set<String> records = new LinkedHashSet<>();
CSVParser parser = new CSVParser(new StringReader(csv1), csvFormat);
for (CSVRecord record : parser) {
removeEntries(record, records);
}
parser = new CSVParser(new StringReader(csv2), csvFormat);
for (CSVRecord record : parser) {
removeEntries(record, records);
}
csvPrinter.printRecord(records);
csvPrinter.flush();
}
} catch (IOException ignored) {
// can't really happen
}
return writer.toString();
}

private static void appendEntries(
CSVRecord record, Set<String> result, Set<String> existingKeys, boolean mergeKeyValues) {
record
Expand All @@ -1404,10 +1341,6 @@ private static void appendEntries(
});
}

private static void removeEntries(CSVRecord record, Set<String> result) {
record.toList().forEach(result::remove);
}

private static Optional<String> getKey(String keyValue) {
if (StringUtils.isEmpty(keyValue)) {
return Optional.empty();
Expand All @@ -1431,62 +1364,6 @@ public static void mergeCSVs(
}
}

public static void checkCSVStrings(
String groupYsqlPgConfCsv, String newYsqlPgConfCsv, GroupName group) {
StringWriter writer = new StringWriter();
CSVFormat csvFormat = CSVFormat.DEFAULT.builder().setRecordSeparator("").build();

try (CSVPrinter csvPrinter = new CSVPrinter(writer, csvFormat)) {
Set<String> existingKeys = new HashSet<>();
Set<String> records = new LinkedHashSet<>();

Map<String, String> groupMap = parseCSVStringToMap(groupYsqlPgConfCsv, csvFormat);
Map<String, String> newMap = parseCSVStringToMap(newYsqlPgConfCsv, csvFormat);

// Compare groupMap with newMap
for (Map.Entry<String, String> entry : groupMap.entrySet()) {
String key = entry.getKey();
String value = entry.getValue();

if (!newMap.containsKey(key)) {
throw new PlatformServiceException(
BAD_REQUEST,
String.format("Key %s from group %s is not present in ysql_pg_conf_csv", key, group));
}

if (!newMap.get(key).equals(value)) {
throw new PlatformServiceException(
BAD_REQUEST,
String.format(
"Value for key %s from group %s does not match in ysql_pg_conf_csv. Have %s found"
+ " %s. Disable the group to edit values.",
key, group, value, newMap.get(key)));
}
}
} catch (IOException ignored) {
// can't really happen
}
}

private static Map<String, String> parseCSVStringToMap(String csv, CSVFormat csvFormat)
throws IOException {
Map<String, String> map = new HashMap<>();
CSVParser parser = new CSVParser(new StringReader(csv), csvFormat);

for (CSVRecord record : parser) {
for (String pair : record) {
String[] keyValue = pair.split("=");
if (keyValue.length == 2) {
map.put(keyValue[0], keyValue[1]);
} else {
throw new IllegalArgumentException("Invalid key-value pair: " + pair);
}
}
}

return map;
}

private static void mergeHostAndPort(
Map<String, String> userGFlags, String addressKey, int port) {
String val = userGFlags.get(addressKey);
Expand Down
8 changes: 4 additions & 4 deletions managed/src/main/java/com/yugabyte/yw/controllers/handlers/UniverseCRUDHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1497,14 +1497,14 @@ public UUID createReadReplicaCluster(
readOnlyCluster.userIntent.providerType = Common.CloudType.valueOf(provider.getCode());
readOnlyCluster.validate(!cloudEnabled, isAuthEnforced, taskParams.nodeDetailsSet);
if (readOnlyCluster.userIntent.specificGFlags != null) {
if (readOnlyCluster.userIntent.specificGFlags.isInheritFromPrimary()) {
SpecificGFlags primaryGFlags = primaryCluster.userIntent.specificGFlags;
if (primaryGFlags != null) {
SpecificGFlags primaryGFlags = primaryCluster.userIntent.specificGFlags;
if (primaryGFlags != null) {
if (readOnlyCluster.userIntent.specificGFlags.isInheritFromPrimary()) {
readOnlyCluster.userIntent.specificGFlags.setPerProcessFlags(
primaryGFlags.getPerProcessFlags());
readOnlyCluster.userIntent.specificGFlags.setPerAZ(primaryGFlags.getPerAZ());
readOnlyCluster.userIntent.specificGFlags.setGflagGroups(primaryGFlags.getGflagGroups());
}
readOnlyCluster.userIntent.specificGFlags.setGflagGroups(primaryGFlags.getGflagGroups());
}
List<Cluster> clusters = new ArrayList<>(universe.getUniverseDetails().clusters);
clusters.add(readOnlyCluster);
Expand Down
15 changes: 10 additions & 5 deletions managed/src/main/java/com/yugabyte/yw/forms/UpgradeWithGFlags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,6 @@ protected void verifyPreviewGFlagsSettings(Universe universe) {
private boolean verifySpecificGFlags(Universe universe) {
// verify changes to groups here
// if groups are added, cannot allow changes to those gflags
Cluster currentPrimaryCluster = universe.getUniverseDetails().getPrimaryCluster();
SpecificGFlags currentSpecificGFlags = new SpecificGFlags();
if (currentPrimaryCluster.userIntent.specificGFlags != null) {
currentSpecificGFlags = currentPrimaryCluster.userIntent.specificGFlags;
}
Map<UUID, Cluster> newClusters =
clusters.stream().collect(Collectors.toMap(c -> c.uuid, c -> c));
boolean hasClustersToUpdate = false;
Expand Down Expand Up @@ -123,6 +118,16 @@ private boolean verifySpecificGFlags(Universe universe) {
}
}
}
// check if gflag groups have changed with NON Restart. Throw error if so
if (newCluster.userIntent.specificGFlags != null
&& curCluster.userIntent.specificGFlags != null
&& !Objects.equals(
newCluster.userIntent.specificGFlags.getGflagGroups(),
curCluster.userIntent.specificGFlags.getGflagGroups())) {
throw new PlatformServiceException(
Http.Status.BAD_REQUEST,
"Gflag groups cannot be changed through non-restart upgrade option.");
}
}
}
return hasClustersToUpdate;
Expand Down
Loading

0 comments on commit e37d791

Please sign in to comment.