Skip to content

Commit

Permalink
[BUG] fix healthcheck logic to expect object and return ids (#2277) (#…
Browse files Browse the repository at this point in the history 
…2300) (#2303)

Original implementation incorrectly assumed the return list of
nodes was an object array.
This PR:
#2232

Addressed the return but didn't catch the nodes.find in the return
which is a function for an array.

Also, refactors to return a list of node_ids because the original
implementation indicated that it should but it can return node ids
but it never did. It only returned `null` or `_local`, the problem
with this approach is that it doesn't expect valid node version
with different DIs or filter out nodes when we pass `null` as the node
ID for the node info call because it was fan out the request to all
nodes.

Now this function will return `_local` if all the nodes share the
same cluster_id using a greedy approach since we can assume it is
all the same version.

Will return node ids, if the cluster_id are different so it will
pass a CSV to the node info call and return the info for those nodes.
And null if no cluster_id is present, ie, fan out the response.

Original issue:
#2203

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
(cherry picked from commit e10ba34)

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>
  • Loading branch information
@opensearch-trigger-bot @kavilla
opensearch-trigger-bot[bot] and kavilla authored Sep 12, 2022
1 parent 1186804 commit 3877f98
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 47 deletions.
32 changes: 20 additions & 12 deletions src/core/server/opensearch/version_check/ensure_opensearch_version.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -254,18 +254,18 @@ describe('pollOpenSearchNodesVersion', () => {
it('returns compatibility results and isCompatible=true with filters', (done) => {
expect.assertions(2);
const target = {
id: '0',
cluster_id: '0',
attribute: 'foo',
};
const filter = {
id: '1',
cluster_id: '1',
attribute: 'bar',
};

// will filter out every odd index
const nodes = createNodesWithAttribute(
target.id,
filter.id,
target.cluster_id,
filter.cluster_id,
target.attribute,
filter.attribute,
'5.1.0',
Expand All @@ -274,14 +274,18 @@ describe('pollOpenSearchNodesVersion', () => {
'5.1.1-Beta1'
);

const filteredNodes = nodes;
delete filteredNodes.nodes['node-1'];
delete filteredNodes.nodes['node-3'];

// @ts-expect-error we need to return an incompatible type to use the testScheduler here
internalClient.cluster.state.mockReturnValueOnce({ body: nodes });

nodeInfosSuccessOnce(nodes);

pollOpenSearchNodesVersion({
internalClient,
optimizedHealthcheck: { id: target.id, filters: { custom_attribute: filter.attribute } },
optimizedHealthcheck: { id: 'cluster_id', filters: { custom_attribute: filter.attribute } },
opensearchVersionCheckInterval: 1,
ignoreVersionMismatch: false,
opensearchDashboardsVersion: OPENSEARCH_DASHBOARDS_VERSION,
Expand All @@ -291,7 +295,7 @@ describe('pollOpenSearchNodesVersion', () => {
.subscribe({
next: (result) => {
expect(result).toEqual(
mapNodesVersionCompatibility(nodes, OPENSEARCH_DASHBOARDS_VERSION, false)
mapNodesVersionCompatibility(filteredNodes, OPENSEARCH_DASHBOARDS_VERSION, false)
);
expect(result.isCompatible).toBe(true);
},
Expand All @@ -303,18 +307,18 @@ describe('pollOpenSearchNodesVersion', () => {
it('returns compatibility results and isCompatible=false with filters', (done) => {
expect.assertions(2);
const target = {
id: '0',
cluster_id: '0',
attribute: 'foo',
};
const filter = {
id: '1',
cluster_id: '1',
attribute: 'bar',
};

// will filter out every odd index
const nodes = createNodesWithAttribute(
target.id,
filter.id,
target.cluster_id,
filter.cluster_id,
target.attribute,
filter.attribute,
'5.1.0',
Expand All @@ -323,14 +327,18 @@ describe('pollOpenSearchNodesVersion', () => {
'5.1.1-Beta1'
);

const filteredNodes = nodes;
delete filteredNodes.nodes['node-1'];
delete filteredNodes.nodes['node-3'];

// @ts-expect-error we need to return an incompatible type to use the testScheduler here
internalClient.cluster.state.mockReturnValueOnce({ body: nodes });

nodeInfosSuccessOnce(nodes);

pollOpenSearchNodesVersion({
internalClient,
optimizedHealthcheck: { id: target.id, filters: { custom_attribute: filter.attribute } },
optimizedHealthcheck: { id: 'cluster_id', filters: { custom_attribute: filter.attribute } },
opensearchVersionCheckInterval: 1,
ignoreVersionMismatch: false,
opensearchDashboardsVersion: OPENSEARCH_DASHBOARDS_VERSION,
Expand All @@ -340,7 +348,7 @@ describe('pollOpenSearchNodesVersion', () => {
.subscribe({
next: (result) => {
expect(result).toEqual(
mapNodesVersionCompatibility(nodes, OPENSEARCH_DASHBOARDS_VERSION, false)
mapNodesVersionCompatibility(filteredNodes, OPENSEARCH_DASHBOARDS_VERSION, false)
);
expect(result.isCompatible).toBe(false);
},
Expand Down
68 changes: 33 additions & 35 deletions src/core/server/opensearch/version_check/ensure_opensearch_version.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

import { timer, of, from, Observable } from 'rxjs';
import { map, distinctUntilChanged, catchError, exhaustMap, mergeMap } from 'rxjs/operators';
import { get } from 'lodash';
import { ApiResponse } from '@elastic/elasticsearch';
import {
opensearchVersionCompatibleWithOpenSearchDashboards,
Expand All @@ -51,67 +50,66 @@ import type { OpenSearchClient } from '../client';
* that is supplied through the healthcheck param. This node attribute is configurable
* in opensearch_dashboards.yml. It can also filter attributes out by key-value pair.
* If all nodes have the same cluster id then we do not fan out the healthcheck and use '_local' node
* If there are multiple cluster ids then we use the default fan out behavior
* If there are multiple cluster ids then we return an array of node ids to check.
* If the supplied node attribute is missing then we return null and use default fan out behavior
* @param {OpenSearchClient} internalClient
* @param {OptimizedHealthcheck} healthcheck
* @returns {string|null} '_local' if all nodes have the same cluster_id, otherwise null
* @returns {string|string[]|null} '_local' if all nodes have the same cluster_id, array of node ids if different cluster_id, null if no cluster_id or nodes returned
*/
export const getNodeId = async (
internalClient: OpenSearchClient,
healthcheck: OptimizedHealthcheck
): Promise<string | null> => {
): Promise<'_local' | string[] | null> => {
try {
// If missing an id, we have nothing to check
if (!healthcheck.id) return null;

let path = `nodes.*.attributes.${healthcheck.id}`;
const filters = healthcheck.filters;
if (filters) {
Object.keys(filters).forEach((key) => {
path += `,nodes.*.attributes.${key}`;
});
const filterKeys = filters ? Object.keys(filters) : [];

for (const key of filterKeys) {
path += `,nodes.*.attributes.${key}`;
}

/*
* Using _cluster/state/nodes to retrieve the cluster_id of each node from cluster manager node which
* is considered to be a lightweight operation to aggegrate different cluster_ids from the OpenSearch nodes.
*/
const state = (await internalClient.cluster.state({