[docdb] yb-master fails to restart after errors during first run (yugabyte#5276)

Summary:
Before we depended on the `is_first_run` flag (which is set upon creation of the instance file) in order to determine how we would initialize the system catalog metadata. Since this flag is not very trustworthy if there are errors between instance file creation and sys catalog init, this revision removes this dependency - Instead we now will do a load or create of the sys catalog metadata.
Also removing temporary fix from yugabyte#4866.

Test Plan:
Test examples from the github issue:
```
# Creation of instance file succeeds but sys catalog creation fails
yb-master --fs_data_dirs=/tmp/testcrash1 --rpc_bind_addresses=127.0.0.1:7100 --master_addresses=127.0.0.2:7100 --replication_factor=1
# Should succeed now:
yb-master --fs_data_dirs=/tmp/testcrash1 --rpc_bind_addresses=127.0.0.1:7100 --master_addresses=127.0.0.1:7100 --replication_factor=1
```
---
Also testing example from issue yugabyte#4866:
```
rm -rf /tmp/foo1
mkdir -p /tmp/foo1
python3 -m http.server 7100 --bind 127.0.0.1 &
yb-master --fs_data_dirs=/tmp/foo1 --rpc_bind_addresses=127.0.0.1:7100 --master_addresses=127.0.0.1:7100 --replication_factor=1
kill %1
yb-master --fs_data_dirs=/tmp/foo1 --rpc_bind_addresses=127.0.0.1:7100 --master_addresses=127.0.0.1:7100 --replication_factor=1
# Still works
```
---
Also adding `ybd --cxx-test master-test --gtest_filter MasterTest.TestNetworkErrorOnFirstRun`

Reviewers: sanketh, bogdan, rahuldesirazu

Reviewed By: rahuldesirazu

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D9240

Author: hulien22

src/yb/master/catalog_manager.cc

@@ -561,7 +561,7 @@ CatalogManager::~CatalogManager() {
   Shutdown();
 }
 
-Status CatalogManager::Init(bool is_first_run) {
+Status CatalogManager::Init() {
   {
     std::lock_guard<simple_spinlock> l(state_lock_);
     CHECK_EQ(kConstructed, state_);
@@ -575,7 +575,7 @@ Status CatalogManager::Init(bool is_first_run) {
   metric_num_tablet_servers_dead_ =
     METRIC_num_tablet_servers_dead.Instantiate(master_->metric_entity_cluster(), 0);
 
-  RETURN_NOT_OK_PREPEND(InitSysCatalogAsync(is_first_run),
+  RETURN_NOT_OK_PREPEND(InitSysCatalogAsync(),
                         "Failed to initialize sys tables async");
 
   if (PREDICT_FALSE(FLAGS_TEST_simulate_slow_system_tablet_bootstrap_secs > 0)) {
@@ -1289,9 +1289,16 @@ Status CatalogManager::CheckLocalHostInMasterAddresses() {
       master_->opts().master_addresses_flag);
 }
 
-Status CatalogManager::InitSysCatalogAsync(bool is_first_run) {
+Status CatalogManager::InitSysCatalogAsync() {
   std::lock_guard<LockType> l(lock_);
-  if (is_first_run) {
+
+  // Optimistically try to load data from disk.
+  Status s = sys_catalog_->Load(master_->fs_manager());
+
+  if (!s.ok() && s.IsNotFound()) {
+    // We are on our first run, need to create the metadata file.
+    LOG(INFO) << "Did not find previous SysCatalogTable data on disk";
+
     if (!master_->opts().AreMasterAddressesProvided()) {
       master_->SetShellMode(true);
       LOG(INFO) << "Starting master in shell mode.";
@@ -1300,10 +1307,11 @@ Status CatalogManager::InitSysCatalogAsync(bool is_first_run) {
 
     RETURN_NOT_OK(CheckLocalHostInMasterAddresses());
     RETURN_NOT_OK(sys_catalog_->CreateNew(master_->fs_manager()));
-  } else {
-    RETURN_NOT_OK(sys_catalog_->Load(master_->fs_manager()));
+
+    return Status::OK();
   }
-  return Status::OK();
+
+  return s;
 }
 
 bool CatalogManager::IsInitialized() const {

src/yb/master/catalog_manager.h

@@ -170,7 +170,7 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
   explicit CatalogManager(Master *master);
   virtual ~CatalogManager();
 
-  CHECKED_STATUS Init(bool is_first_run);
+  CHECKED_STATUS Init();
 
   void Shutdown();
   CHECKED_STATUS CheckOnline() const;
@@ -826,7 +826,7 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
   // sys_catalog_.
   //
   // This method is thread-safe.
-  CHECKED_STATUS InitSysCatalogAsync(bool is_first_run);
+  CHECKED_STATUS InitSysCatalogAsync();
 
   // Helper for creating the initial TableInfo state
   // Leaves the table "write locked" with the new info in the

src/yb/master/master-test.cc

@@ -67,6 +67,7 @@ DECLARE_bool(TEST_hang_on_namespace_transition);
 DECLARE_bool(TEST_simulate_crash_after_table_marked_deleting);
 DECLARE_int32(TEST_sys_catalog_write_rejection_percentage);
 DECLARE_bool(TEST_tablegroup_master_only);
+DECLARE_bool(TEST_simulate_port_conflict_error);
 
 namespace yb {
 namespace master {
@@ -1804,6 +1805,18 @@ TEST_F(MasterTest, TestFailedMasterRestart) {
   ASSERT_OK(mini_master_->Start());
 }
 
+TEST_F(MasterTest, TestNetworkErrorOnFirstRun) {
+  TearDown();
+  mini_master_.reset(new MiniMaster(Env::Default(), GetTestPath("Master-test"),
+                                    AllocateFreePort(), AllocateFreePort(), 0));
+  FLAGS_TEST_simulate_port_conflict_error = true;
+  ASSERT_NOK(mini_master_->Start());
+  // Instance file should be properly initialized, but consensus metadata is not initialized.
+  FLAGS_TEST_simulate_port_conflict_error = false;
+  // Restarting master should succeed.
+  ASSERT_OK(mini_master_->Start());
+}
+
 static void GetTableSchema(const char* table_name,
                            const char* namespace_name,
                            const Schema* kSchema,

src/yb/master/master.cc

@@ -273,7 +273,7 @@ Status Master::InitCatalogManager() {
   if (catalog_manager_->IsInitialized()) {
     return STATUS(IllegalState, "Catalog manager is already initialized");
   }
-  RETURN_NOT_OK_PREPEND(catalog_manager_->Init(is_first_run_),
+  RETURN_NOT_OK_PREPEND(catalog_manager_->Init(),
                         "Unable to initialize catalog manager");
   return Status::OK();
 }

src/yb/server/server_base.cc

@@ -67,6 +67,7 @@
 #include "yb/util/monotime.h"
 #include "yb/util/net/sockaddr.h"
 #include "yb/util/net/net_util.h"
+#include "yb/util/status.h"
 #include "yb/util/user.h"
 #include "yb/util/pb_util.h"
 #include "yb/util/rolling_log.h"
@@ -98,6 +99,9 @@ DEFINE_bool(TEST_check_broadcast_address, true, "Break connectivity in test mini
 
 DEFINE_test_flag(string, public_hostname_suffix, ".ip.yugabyte", "Suffix for public hostnames.");
 
+DEFINE_test_flag(bool, simulate_port_conflict_error, false,
+                 "Simulate a port conflict error during initialization.");
+
 using namespace std::literals;
 using namespace std::placeholders;
 
@@ -157,7 +161,6 @@ RpcServerBase::RpcServerBase(string name, const ServerBaseOptions& options,
       metric_registry_(new MetricRegistry()),
       metric_entity_(METRIC_ENTITY_server.Instantiate(metric_registry_.get(),
                                                       metric_namespace)),
-      is_first_run_(false),
       options_(options),
       initialized_(false),
       stop_metrics_logging_latch_(1) {
@@ -461,21 +464,17 @@ Status RpcAndWebServerBase::Init() {
   if (s.IsNotFound() || (!s.ok() && fs_manager_->HasAnyLockFiles())) {
     LOG(INFO) << "Could not load existing FS layout: " << s.ToString();
     LOG(INFO) << "Creating new FS layout";
-    is_first_run_ = true;
     RETURN_NOT_OK_PREPEND(fs_manager_->CreateInitialFileSystemLayout(true),
                           "Could not create new FS layout");
     s = fs_manager_->Open();
   }
   RETURN_NOT_OK_PREPEND(s, "Failed to load FS layout");
 
-  s = RpcServerBase::Init();
-  if (!s.ok() && is_first_run_) {
-    // TODO (julien) : Remove this once #5276 is fixed.
-    LOG(ERROR) << "Encountered an error, deleting FS files in order to reset run state: " << s;
-    RETURN_NOT_OK_PREPEND(fs_manager_->DeleteFileSystemLayout(),
-                          "Failed deleting FS layout after RPCServerBase init failed.");
+  if (PREDICT_FALSE(FLAGS_TEST_simulate_port_conflict_error)) {
+    return STATUS(NetworkError, "Simulated port conflict error");
   }
-  RETURN_NOT_OK_PREPEND(s, "Failed to initialize FS layout");
+
+  RETURN_NOT_OK(RpcServerBase::Init());
 
   return Status::OK();
 }

src/yb/server/server_base.h

@@ -130,7 +130,6 @@ class RpcServerBase {
   gscoped_ptr<RpcServer> rpc_server_;
   std::unique_ptr<rpc::Messenger> messenger_;
   std::unique_ptr<rpc::ProxyCache> proxy_cache_;
-  bool is_first_run_;
 
   scoped_refptr<Clock> clock_;