HPCC-30299 Update secrets in the background to avoid roxie stalls #18071

ghalliday · 2023-11-22T14:40:07Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2023-11-22T14:40:27Z

https://track.hpccsystems.com/browse/HPCC-30299
Jira updated

ghalliday · 2023-11-22T14:41:32Z

This is easiest to review as two separate commits - one which refactors the code and rearranges it. The second implements the background updater and fixes problems with access times not being updated.

ghalliday · 2023-11-22T14:58:04Z

Worth checking the unit tests are correct, and also that the timeouts are sensible.
By default it will check for updates every 30 seconds (timeout/20) , on any items that are due to need refreshing in the next 2 minutes (timeout/5) .

richardkchapman

Some minor comments, plus a question on the logic

richardkchapman · 2023-11-22T16:02:50Z

system/jlib/jsecrets.cpp

+{
+    const char * slash = strchr(key, '/');
+    assertex(slash);
+    const char * at = strchr(slash, '@');


Will go wrong if anyone put a # into a vault id. Perhaps that is checked elsewhere

Agree. The vault id is just the name of the vault config entry in values.yaml or environment.xml. I don't think there is currently any checking for allowed characters. We have control over what is allowed and probably should limit the characters used. Could add checking to the helm chart and the CVault constructor.

Added HPCC-30124 to check them.

richardkchapman · 2023-11-22T16:04:26Z

system/jlib/jsecrets.cpp

+    const char * at = strchr(slash, '@');
+    const char * hash = strchr(slash, '#');
+
+    const char * end = nullptr;


Feels like this code would be simpler if you initialised end = key+strlen(key) rather than null

It would be, not sure I want to change it no though.

richardkchapman · 2023-11-22T16:09:34Z

system/jlib/jsecrets.cpp

    // We should never replace known contents for unknown contents
    // so once this returns true it should always return true
    bool hasContents() const
    {
        return contents != nullptr;
    }

+    //Has the secret value been used since it was last checked for an update?


I'm struggling to understand why it is relevant whether a secret value has been used recently, when updating them. Unless you are removing secrets from the cache if not used recently (which I don't think you are).

The aim is to not refresh secrets in the background that have not been used recently - to avoid unnecessary load on the vault.

richardkchapman · 2023-11-22T16:11:46Z

system/jlib/jsecrets.cpp

+        for (auto & entry : secrets)
+        {
+            SecretCacheEntry * secret = entry.second.get();
+            if (secret->isActive() && secret->needsRefresh(when))


This doesn't feel right - just because a secret has not been read recently doesn't mean that the next time it IS used it's ok to use an old version, does it?

Next time it is actually used it will check to see if it needs a refresh, and if it does it will go and get the new value. This PR doesn't change that behaviour, it preemptively refreshes secrets that are in active use behind the scenes to avoid roxie pausing when it needs to access them.

AttilaVamos · 2023-11-23T12:18:17Z

I found a bunch of core files (in CentOS based On-Demand Smoketest) and the generated trace files show similar problem:

Core was generated by `hthor --workunit=W20231123-104712-1 --daliServers=10.22.254.118:7070'.
Program terminated with signal SIGABRT, Aborted.
#0  0x00007f8a3e4df387 in raise () from /lib64/libc.so.6

Backtrace for all threads
==========================
Thread 1 (Thread 0x7f8a3cb1f300 (LWP 15963)):
#0  0x00007f8a3e4df387 in raise () from /lib64/libc.so.6
#1  0x00007f8a3e4e0a78 in abort () from /lib64/libc.so.6
#2  0x00007f8a3edf0a95 in __gnu_cxx::__verbose_terminate_handler() () from /lib64/libstdc++.so.6
#3  0x00007f8a3edeea06 in ?? () from /lib64/libstdc++.so.6
#4  0x00007f8a3eded9b9 in ?? () from /lib64/libstdc++.so.6
#5  0x00007f8a3edee624 in __gxx_personality_v0 () from /lib64/libstdc++.so.6
#6  0x00007f8a3e8868e3 in ?? () from /lib64/libgcc_s.so.1
#7  0x00007f8a3e886e17 in _Unwind_Resume () from /lib64/libgcc_s.so.1
#8  0x00007f8a4138d8c2 in CInterfaceOf<ISpan>::Release() const () from /opt/HPCCSystems/lib/libjlib.so
#9  0x00007f8a41291041 in DummyLogCtx::~DummyLogCtx() () from /opt/HPCCSystems/lib/libjlib.so
#10 0x00007f8a3e4e305a in __cxa_finalize () from /lib64/libc.so.6
#11 0x00007f8a4123daa7 in ?? () from /opt/HPCCSystems/lib/libjlib.so
#12 0x00007fff042b6aa0 in ?? ()
#13 0x00007f8a44c7708a in _dl_fini () from /lib64/ld-linux-x86-64.so.2
Backtrace stopped: frame did not save the PC

 Registers:
==========================
rax            0x0                 0
rbx            0x7f8a3e871868      140231731255400
rcx            0xffffffffffffffff  -1
rdx            0x6                 6
rsi            0x3e5b              15963
rdi            0x3e5b              15963
rbp            0x403148            0x403148 <typeinfo name for IException*>
rsp            0x7fff042b6078      0x7fff042b6078
r8             0x7fff042b5cb0      140733263338672
r9             0x4                 4
r10            0x8                 8
r11            0x202               514
r12            0x575590            5723536
r13            0x0                 0
r14            0x0                 0
r15            0x0                 0
rip            0x7f8a3e4df387      0x7f8a3e4df387 <raise+55>
eflags         0x202               [ IF ]
cs             0x33                51
ss             0x2b                43
ds             0x0                 0
es             0x0                 0
fs             0x0                 0
gs             0x0                 0

 Disas:
==========================
Dump of assembler code for function raise:
   0x00007f8a3e4df350 <+0>:	mov    %fs:0x2d4,%ecx
   0x00007f8a3e4df358 <+8>:	mov    %fs:0x2d0,%esi
   0x00007f8a3e4df360 <+16>:	test   %esi,%esi
   0x00007f8a3e4df362 <+18>:	jne    0x7f8a3e4df398 <raise+72>
   0x00007f8a3e4df364 <+20>:	mov    $0xba,%eax
   0x00007f8a3e4df369 <+25>:	syscall 
   0x00007f8a3e4df36b <+27>:	mov    %eax,%ecx
   0x00007f8a3e4df36d <+29>:	mov    %eax,%fs:0x2d0
   0x00007f8a3e4df375 <+37>:	mov    %eax,%esi
   0x00007f8a3e4df377 <+39>:	movslq %edi,%rdx
   0x00007f8a3e4df37a <+42>:	movslq %esi,%rsi
   0x00007f8a3e4df37d <+45>:	movslq %ecx,%rdi
   0x00007f8a3e4df380 <+48>:	mov    $0xea,%eax
   0x00007f8a3e4df385 <+53>:	syscall 
=> 0x00007f8a3e4df387 <+55>:	cmp    $0xfffffffffffff000,%rax
   0x00007f8a3e4df38d <+61>:	ja     0x7f8a3e4df3ad <raise+93>
   0x00007f8a3e4df38f <+63>:	repz ret 
   0x00007f8a3e4df391 <+65>:	nopl   0x0(%rax)
   0x00007f8a3e4df398 <+72>:	test   %ecx,%ecx
   0x00007f8a3e4df39a <+74>:	jg     0x7f8a3e4df377 <raise+39>
   0x00007f8a3e4df39c <+76>:	mov    %ecx,%eax
   0x00007f8a3e4df39e <+78>:	neg    %eax
   0x00007f8a3e4df3a0 <+80>:	and    $0x7fffffff,%ecx
   0x00007f8a3e4df3a6 <+86>:	cmove  %esi,%eax
   0x00007f8a3e4df3a9 <+89>:	mov    %eax,%ecx
   0x00007f8a3e4df3ab <+91>:	jmp    0x7f8a3e4df377 <raise+39>
   0x00007f8a3e4df3ad <+93>:	mov    0x390a9c(%rip),%rdx        # 0x7f8a3e86fe50
   0x00007f8a3e4df3b4 <+100>:	neg    %eax
   0x00007f8a3e4df3b6 <+102>:	mov    %eax,%fs:(%rdx)
   0x00007f8a3e4df3b9 <+105>:	or     $0xffffffffffffffff,%rax
   0x00007f8a3e4df3bd <+109>:	ret    
End of assembler dump.

I don't know it is related to your changes or not.
If not I will raise a JIRA for it.

mckellyln · 2023-12-05T18:00:13Z

system/jlib/jsecrets.cpp

+    if (resolved)
+        globalSecretCache.updateSecret(secret, resolved, now, accessed);
+    else
+        secret->noteFailedUpdate(now, accessed);


would we always honor accessed if not resolved ?

mckellyln · 2023-12-05T18:02:34Z

system/jlib/jsecrets.cpp

+    const char * slash = strchr(key, '/');
+    assertex(slash);
+    const char * at = strchr(slash, '@');
+    const char * hash = strchr(slash, '#');


Yes - are these '/', '#', '@' special chars always ok to use as separators ?

mckellyln

I have two minor comments, but otherwise looks good to me.

afishbeck

Very minor comments otherwise looks good. Vault name should be restricted to basic charset and validated in helm chart and perhaps elsewhere.

afishbeck · 2023-12-12T23:26:34Z

system/jlib/jsecrets.cpp

+{
+    const char * slash = strchr(key, '/');
+    assertex(slash);
+    const char * at = strchr(slash, '@');


Agree. The vault id is just the name of the vault config entry in values.yaml or environment.xml. I don't think there is currently any checking for allowed characters. We have control over what is allowed and probably should limit the characters used. Could add checking to the helm chart and the CVault constructor.

afishbeck · 2023-12-12T23:46:16Z

testing/unittests/jlibtests.cpp

+        MilliSleep(30); // elapsed=180 = 80 + 80 + 20
+        CPPUNIT_ASSERT(secret6->isValid());
+        CPPUNIT_ASSERT(!secret6->isStale());
+        unsigned version3 = secret6->getVersion(); // Mark the value as accessed, but too early to be refreshed


Is the above comment wrong? No longer too early to be refreshed?

You are correct comment is wrong.

ghalliday · 2023-12-15T11:53:35Z

@afishbeck I have created a jira, fixed the comment and squashed.
What do you think is the best target for this. Is it important enough to go in 9.4.x? Without it we may get periodic pauses accessing secrets in roxie queries.

afishbeck · 2023-12-15T14:36:48Z

@afishbeck I have created a jira, fixed the comment and squashed. What do you think is the best target for this. Is it important enough to go in 9.4.x? Without it we may get periodic pauses accessing secrets in roxie queries.

I agree that this is an important improvement. One way of looking at it is that since this can be turned off via config, what is the chance the code change would break something even if turned off?

ghalliday · 2023-12-19T14:15:46Z

@mckellyln I decided not to merge because I was concerned about msTick() wrapping. In this case, secrets are never removed from the hash table - which means that if a roxie has been up for more than 25 days, and early in its life it accessed a secret, but hasn't since, the test for needs refresh and expired may be wrong.
I think the only ill effect is that isActive() will start returning true - so all secrets will be requested from the vault every 25 days, but switching to nsTick() avoids the problem for 250 years.
Please can you check the update - the unit tests caught several mistakes.

mckellyln · 2023-12-20T14:59:08Z

system/jlib/jsecrets.cpp

 }

 extern jlib_decl void setSecretTimeout(unsigned timeoutMs)
 {
-    secretTimeoutMs = timeoutMs;
+    secretTimeoutNs = (unsigned __int64)timeoutMs * 1000000;


Why unsigned __int64 and not __uint64 ? Does it matter ?

mckellyln

Commit 2 looks good.
Approved.

Signed-off-by: Gavin Halliday <[email protected]>

ghalliday requested a review from afishbeck November 22, 2023 14:40

ghalliday requested a review from mckellyln November 22, 2023 14:58

ghalliday force-pushed the issue30299 branch from 15fea4b to c692f35 Compare November 22, 2023 15:44

richardkchapman reviewed Nov 22, 2023

View reviewed changes

mckellyln reviewed Dec 5, 2023

View reviewed changes

mckellyln approved these changes Dec 5, 2023

View reviewed changes

afishbeck reviewed Dec 12, 2023

View reviewed changes

richardkchapman approved these changes Dec 13, 2023

View reviewed changes

ghalliday force-pushed the issue30299 branch from c692f35 to 8956ab2 Compare December 15, 2023 11:52

ghalliday force-pushed the issue30299 branch from 8956ab2 to 42782ff Compare December 19, 2023 11:50

ghalliday requested a review from mckellyln December 19, 2023 14:09

mckellyln reviewed Dec 20, 2023

View reviewed changes

mckellyln approved these changes Dec 20, 2023

View reviewed changes

HPCC-30299 Update secrets in the background to avoid roxie stalls

3a50fc1

Signed-off-by: Gavin Halliday <[email protected]>

ghalliday force-pushed the issue30299 branch from 3f62ab8 to 3a50fc1 Compare January 3, 2024 16:39

ghalliday merged commit ababf40 into hpcc-systems:candidate-9.4.x Jan 3, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30299 Update secrets in the background to avoid roxie stalls #18071

HPCC-30299 Update secrets in the background to avoid roxie stalls #18071

ghalliday commented Nov 22, 2023 •

edited

Loading

github-actions bot commented Nov 22, 2023

ghalliday commented Nov 22, 2023

ghalliday commented Nov 22, 2023

richardkchapman left a comment

richardkchapman Nov 22, 2023

afishbeck Dec 12, 2023

ghalliday Dec 15, 2023

richardkchapman Nov 22, 2023

ghalliday Dec 15, 2023

richardkchapman Nov 22, 2023

ghalliday Nov 24, 2023

richardkchapman Nov 22, 2023

ghalliday Nov 24, 2023

AttilaVamos commented Nov 23, 2023 •

edited

Loading

mckellyln Dec 5, 2023

mckellyln Dec 5, 2023

mckellyln left a comment

afishbeck left a comment

afishbeck Dec 12, 2023

afishbeck Dec 12, 2023

ghalliday Dec 15, 2023

ghalliday commented Dec 15, 2023

afishbeck commented Dec 15, 2023

ghalliday commented Dec 19, 2023

mckellyln Dec 20, 2023

mckellyln left a comment

HPCC-30299 Update secrets in the background to avoid roxie stalls #18071

HPCC-30299 Update secrets in the background to avoid roxie stalls #18071

Conversation

ghalliday commented Nov 22, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 22, 2023

ghalliday commented Nov 22, 2023

ghalliday commented Nov 22, 2023

richardkchapman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AttilaVamos commented Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mckellyln left a comment

Choose a reason for hiding this comment

afishbeck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday commented Dec 15, 2023

afishbeck commented Dec 15, 2023

ghalliday commented Dec 19, 2023

Choose a reason for hiding this comment

mckellyln left a comment

Choose a reason for hiding this comment

ghalliday commented Nov 22, 2023 •

edited

Loading

AttilaVamos commented Nov 23, 2023 •

edited

Loading