Skip to content

HBASE-28050 RSProcedureDispatcher to fail-fast for krb auth failures #5391

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.

Sign up for GitHub

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

Merged
virajjasani merged 10 commits into from
Sep 29, 2023
Merged

HBASE-28050 RSProcedureDispatcher to fail-fast for krb auth failures #5391

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a5bc01
HBASE-28050 RSProcedureDispatcher to fail-fast for SaslException
virajjasani Sep 11, 2023
a61c49f
addendum
virajjasani Sep 12, 2023
4a79223
addendum
virajjasani Sep 12, 2023
8549505
addendum
virajjasani Sep 13, 2023
a8ba2ee
addendum
virajjasani Sep 14, 2023
420fcb6
addendum
virajjasani Sep 14, 2023
8b59b13
addendum
virajjasani Sep 14, 2023
6fbfe4d
addendum
virajjasani Sep 15, 2023
fbea61e
addendum
virajjasani Sep 15, 2023
a8626fa
addendum
virajjasani Sep 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion hbase-client/src/main/java/org/apache/hadoop/hbase/ipc/NettyRpcConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.io.crypto.tls.X509Util;
import org.apache.hadoop.hbase.ipc.BufferCallBeforeInitHandler.BufferCallEvent;
import org.apache.hadoop.hbase.ipc.HBaseRpcController.CancellationCallback;
Expand Down Expand Up @@ -347,7 +348,7 @@ public void operationComplete(ChannelFuture future) throws Exception {
private void sendRequest0(Call call, HBaseRpcController hrc) throws IOException {
assert eventLoop.inEventLoop();
if (reloginInProgress) {
throw new IOException("Can not send request because relogin is in progress.");
throw new IOException(HConstants.RELOGIN_IS_IN_PROGRESS);
Copy link
Contributor

@apurtell apurtell Sep 12, 2023

Choose a reason for hiding this comment

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

This is weird, please don't put these kinds of constants in HConstants. There are too many unrelated concerns there already.

Public static string constant in some other file, even this one, is preferred.

Copy link
Contributor Author

@virajjasani virajjasani Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

NettyRpcConnection is package private, hence can't be accessed from hbase-server

Copy link
Contributor Author

@virajjasani virajjasani Sep 13, 2023

Choose a reason for hiding this comment

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

not sure what is the best place to keep this, anywhere else in hbase-common would also work

Copy link
Contributor

@Apache9 Apache9 Sep 14, 2023

Choose a reason for hiding this comment

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

Please do not put it in HConstants, it is IA.Public.

Copy link
Contributor Author

@virajjasani virajjasani Sep 14, 2023

Choose a reason for hiding this comment

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

i understand but i am not sure what is the best place to keep this in

Copy link
Contributor Author

@virajjasani virajjasani Sep 14, 2023

Choose a reason for hiding this comment

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

ok, this is now taken care of

}
hrc.notifyOnCancel(new RpcCallback<Object>() {

Expand Down
3 changes: 3 additions & 0 deletions hbase-common/src/main/java/org/apache/hadoop/hbase/HConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ public final class HConstants {
/** Just an array of bytes of the right size. */
public static final byte[] HFILEBLOCK_DUMMY_HEADER = new byte[HFILEBLOCK_HEADER_SIZE];

public static final String RELOGIN_IS_IN_PROGRESS =
"Can not send request because relogin is in progress.";

// End HFileBlockConstants.

/**
Expand Down
52 changes: 52 additions & 0 deletions ...-server/src/main/java/org/apache/hadoop/hbase/master/procedure/RSProcedureDispatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import javax.security.sasl.SaslException;
import org.apache.hadoop.hbase.CallQueueTooBigException;
import org.apache.hadoop.hbase.DoNotRetryIOException;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.ServerName;
import org.apache.hadoop.hbase.client.AsyncRegionServerAdmin;
import org.apache.hadoop.hbase.client.RegionInfo;
Expand Down Expand Up @@ -306,6 +308,10 @@ private boolean scheduleForRetry(IOException e) {
serverName, e.toString(), numberOfAttemptsSoFar);
return false;
}
if (isSaslError(e) && numberOfAttemptsSoFar == 0) {
Copy link
Contributor

@apurtell apurtell Sep 12, 2023

Choose a reason for hiding this comment

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

Does it matter how many attempts we have had so far if now we are getting a SASL error?

Copy link
Contributor Author

@virajjasani virajjasani Sep 12, 2023

Choose a reason for hiding this comment

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

Duo's point is that, if some attempts were already successful, it might have triggered region transition already and we might be in the middle of another sub-procedure when we encounter this.

I also think num of attempts should not matter as we won't be able to make any progress anyways, but then it takes our discussion back to the parent Jira HBASE-28048

Copy link
Contributor

@Apache9 Apache9 Sep 13, 2023

Choose a reason for hiding this comment

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

Yes, exactly as @virajjasani said, we need to make sure that the remote procedure has not been sent to the rs then we are safe to quit and choose another rs, otherwise the only safe way is to rely on SCP to tell us the rs is dead so we are safe to quit here.

So if we hit another error, like connection timed out the first time, then we are not sure whether we have already send the procedure to the rs, then no matter what the exceptions are in the following retries, we are not safe to quit.

In the real world, if authentication is not configured correctly, then it is likely we will get sasl error at the first try, so the code is enough to cover the problem here.

Copy link
Contributor

@Apache9 Apache9 Sep 13, 2023

Choose a reason for hiding this comment

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

@virajjasani Better to introduce a method may be called

boolean hasNotReachedRegionServerYet(IOException e)

In this method we could test for both sasl error and call queue too big, and also other exception types in the future. So we do not need to add extra condition every time when we want to add new exception type tests in scheduleForRetry method.

Copy link
Contributor Author

@virajjasani virajjasani Sep 13, 2023

Choose a reason for hiding this comment

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

done

LOG.warn("{} is not reachable; give up after first attempt", serverName, e);
return false;
}
if (e instanceof RegionServerAbortedException || e instanceof RegionServerStoppedException) {
// A better way is to return true here to let the upper layer quit, and then schedule a
// background task to check whether the region server is dead. And if it is dead, call
Expand All @@ -330,6 +336,52 @@ private boolean scheduleForRetry(IOException e) {
return true;
}

private boolean isSaslError(IOException e) {
if (
e instanceof SaslException
|| (e.getMessage() != null && e.getMessage().contains(HConstants.RELOGIN_IS_IN_PROGRESS))
) {
return true;
}
// check 4 level of cause
Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

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

Use a for loop here? And why only test 4 levels?

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

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

it's based on the examples we have seen so far, e.g.
procedure.RSProcedureDispatcher - request to rs1,61020,1692930044498 failed due to java.io.IOException: Call to address=rs1:61020 failed on local exception: java.io.IOException: org.apache.hbase.thirdparty.io.netty.handler.codec.DecoderException: org.apache.hadoop.ipc.RemoteException(javax.security.sasl.SaslException): GSS initiate failed, try=0, retrying...

Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

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

I think we could just use a loop to get the cause until cause is null, to check all the exceptions on chain. And we also need to handle RemoteException specially, to unwrap it instead of just calling getCause?

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

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

handle RemoteException specially, to unwrap it instead of just calling getCause

yes, that is taken care of:

    private boolean isThrowableOfTypeSasl(Throwable cause) {
      if (cause instanceof IOException) {
        IOException unwrappedException = unwrapException((IOException) cause);
        return unwrappedException instanceof SaslException
          || (unwrappedException.getMessage() != null && unwrappedException.getMessage()
            .contains(RpcConnectionConstants.RELOGIN_IS_IN_PROGRESS));
      }
      return false;
    }

Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

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

I mean after unwraping, you still need to go back to the get cause loop, not only test one time...

Copy link
Contributor Author

@virajjasani virajjasani Sep 15, 2023

Choose a reason for hiding this comment

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

Yes, it is in the loop

      while (true) {
        cause = cause.getCause();
        if (cause == null) {
          return false;
        }
        if (isThrowableOfTypeSasl(cause)) {
          return true;
        }
      }

isThrowableOfTypeSasl does the unwrap and checks for type of exception.

Throwable cause = e.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
if (isSaslError(cause)) {
return true;
}
cause = cause.getCause();
if (cause == null) {
return false;
}
return isSaslError(cause);
}

private boolean isSaslError(Throwable cause) {
Copy link
Contributor

@Apache9 Apache9 Sep 15, 2023

Choose a reason for hiding this comment

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

Please do not use the same method name here, as IOException is also a Throwable, although this is valid in Java, but it will confuse the developers.

if (cause instanceof IOException) {
IOException unwrappedException = unwrapException((IOException) cause);
return unwrappedException instanceof SaslException
|| (unwrappedException.getMessage() != null
&& unwrappedException.getMessage().contains(HConstants.RELOGIN_IS_IN_PROGRESS));
}
return false;
}

private long getMaxWaitTime() {
if (this.maxWaitTime < 0) {
// This is the max attempts, not retries, so it should be at least 1.
Expand Down