shill: Service: Track DHCP failures and inform ShouldUseMinimalDHCPConfig()
The default DHCP configuration used by ChromeOS requests a
large number of options in order to support features like
Web Proxy Auto-Discovery. Unfortunately this practice
sometimes runs afoul of issues in some network topologies.
In some networks, a large number of option values in the
DHCP response can cause MTU problems with proxies or other
network elements. This may prevent the reply from being
forwarded back to the client.
Although the problem may not lie with the outgoing request,
it may be possible to mitigate this issue by modifying these
requests. This CL provides a means for the Service to track
such failures and infer from a series of experiments whether
it is likely to be due to an MTU issue. To do this, the
Device informs the selected Service of each DHCP success and
failure, and queries the Service before starting each DHCP
session to inquire whether it should request an extensive or
minimal DHCP options from the server.
In order to detect and respond to such issues, this CL
maintains state in the Service about how DHCP has been
performing. If there have been a spate of recent DHCP
failures, we should suspect that this may be due to the
number of options we are requesting from the DHCP server.
We should confirm that this is in fact the issue by testing
whether a request for a smaller response succeeds. If this
request succeeds we can consider the hunch confirmed and the
client should switch to using small requests for some period
of time. If our test request fails, we can assume for now
that the problem isn't realated to a DHCP response size and
return to the default behavior.
If we have confirmed our hunch earlier and the time period
expires, we should try again to use a more comprehensive
DHCP request. If this succeeds, we can assume either the
network infrastructure has repaired itself or that the
previous hunch was in error, and return to the "not
detected" state. If it fails, we should confirm whether
this is the identical problem to before by re-testing a
small DHCP request size. Since the "confirmed" state did
not pay attention to DHCP failures, it's possible that they
have been failing across the board as of late. If indeed
both large and small DHCP responses fail to reach us, we
should put off our re-test until we start receiving DHCP
replies again.
The state machine implemented for DHCP failures is
illustrated in state machine diagram below (all events not
shown do not cause a state change):
[ Not Detected (send full request) ] <------------
| ^ |
| | |
n * failure failure |
| | |
V | |
[ Suspected (send minimal request) ] |
| |
success |
| |
V |
[ Confirmed (send minimal request) ] |
^ | |
| hold timer elapsed |
| | |
success V |
| [ Retest Full Request ] ----success--/
| | ^
| failure |
| | |
| V |
[ Retest Minimal Request ] |
| success
failure |
| |
V |
[ Retest With No Reply (send minimal requests) ]
The Service only persists two states: "Not Detected" and
"Confirmed". This is done via the presence or absence of
the stored "LastDHCPOptionFailure" property, which is the
time the system last entered the "Confirmed" state. A
third state, "Reset Full Request" is implicitly persisted
if this timestamp is old enough that the hold timer has
expired.
BUG=chromium:297607
TEST=Unit tests
Change-Id: I1ee83debf4d11f25678fe3586574ec04f254a83f
Reviewed-on: https://chromium-review.googlesource.com/174634
Reviewed-by: Paul Stewart <pstew@chromium.org>
Commit-Queue: Paul Stewart <pstew@chromium.org>
Tested-by: Paul Stewart <pstew@chromium.org>
diff --git a/device_unittest.cc b/device_unittest.cc
index ebb4b75..f7f68e4 100644
--- a/device_unittest.cc
+++ b/device_unittest.cc
@@ -311,23 +311,23 @@
SelectService(service);
EXPECT_TRUE(device_->selected_service_.get() == service.get());
- EXPECT_CALL(*service.get(), SetState(Service::kStateConfiguring));
+ EXPECT_CALL(*service, SetState(Service::kStateConfiguring));
device_->SetServiceState(Service::kStateConfiguring);
- EXPECT_CALL(*service.get(), SetFailure(Service::kFailureOutOfRange));
+ EXPECT_CALL(*service, SetFailure(Service::kFailureOutOfRange));
device_->SetServiceFailure(Service::kFailureOutOfRange);
// Service should be returned to "Idle" state
- EXPECT_CALL(*service.get(), state())
+ EXPECT_CALL(*service, state())
.WillOnce(Return(Service::kStateUnknown));
- EXPECT_CALL(*service.get(), SetState(Service::kStateIdle));
- EXPECT_CALL(*service.get(), SetConnection(IsNullRefPtr()));
+ EXPECT_CALL(*service, SetState(Service::kStateIdle));
+ EXPECT_CALL(*service, SetConnection(IsNullRefPtr()));
SelectService(NULL);
// A service in the "Failure" state should not be reset to "Idle"
SelectService(service);
- EXPECT_CALL(*service.get(), state())
+ EXPECT_CALL(*service, state())
.WillOnce(Return(Service::kStateFailure));
- EXPECT_CALL(*service.get(), SetConnection(IsNullRefPtr()));
+ EXPECT_CALL(*service, SetConnection(IsNullRefPtr()));
SelectService(NULL);
}
@@ -338,8 +338,9 @@
metrics(),
manager()));
SelectService(service);
- EXPECT_CALL(*service.get(), DisconnectWithFailure(Service::kFailureDHCP, _));
- EXPECT_CALL(*service.get(), SetConnection(IsNullRefPtr()));
+ EXPECT_CALL(*service, OnDHCPFailure());
+ EXPECT_CALL(*service, DisconnectWithFailure(Service::kFailureDHCP, _));
+ EXPECT_CALL(*service, SetConnection(IsNullRefPtr()));
OnIPConfigUpdated(NULL, false);
}
@@ -352,8 +353,11 @@
SelectService(service);
service->static_ip_parameters_.args_.SetString(kAddressProperty, "1.1.1.1");
service->static_ip_parameters_.args_.SetInt(kPrefixlenProperty, 16);
- EXPECT_CALL(*service.get(), SetState(_)).Times(0);
- EXPECT_CALL(*service.get(), SetConnection(_)).Times(0);
+ // Even though we won't call DisconnectWithFailure, we should still have
+ // the service learn from the failed DHCP attempt.
+ EXPECT_CALL(*service, OnDHCPFailure());
+ EXPECT_CALL(*service, DisconnectWithFailure(_, _)).Times(0);
+ EXPECT_CALL(*service, SetConnection(_)).Times(0);
OnIPConfigUpdated(NULL, false);
}
@@ -366,13 +370,14 @@
SelectService(service);
scoped_refptr<MockIPConfig> ipconfig = new MockIPConfig(control_interface(),
kDeviceName);
- EXPECT_CALL(*service.get(), SetState(Service::kStateConnected));
- EXPECT_CALL(*service.get(), IsConnected())
+ EXPECT_CALL(*service, SetState(Service::kStateConnected));
+ EXPECT_CALL(*service, IsConnected())
.WillRepeatedly(Return(true));
- EXPECT_CALL(*service.get(), IsPortalDetectionDisabled())
+ EXPECT_CALL(*service, IsPortalDetectionDisabled())
.WillRepeatedly(Return(true));
- EXPECT_CALL(*service.get(), SetState(Service::kStateOnline));
- EXPECT_CALL(*service.get(), SetConnection(NotNullRefPtr()));
+ EXPECT_CALL(*service, SetState(Service::kStateOnline));
+ EXPECT_CALL(*service, OnDHCPSuccess());
+ EXPECT_CALL(*service, SetConnection(NotNullRefPtr()));
OnIPConfigUpdated(ipconfig.get(), true);
}
@@ -515,7 +520,7 @@
manager()));
SelectService(service);
- EXPECT_CALL(*service.get(), state()).
+ EXPECT_CALL(*service, state()).
WillRepeatedly(Return(Service::kStateConnected));
EXPECT_CALL(*dynamic_cast<DeviceMockAdaptor *>(device_->adaptor_.get()),
EmitBoolChanged(kPoweredProperty, false));
@@ -623,10 +628,10 @@
MockLinkMonitor *link_monitor = new StrictMock<MockLinkMonitor>();
SetLinkMonitor(link_monitor); // Passes ownership.
SetManager(&manager);
- EXPECT_CALL(*service.get(), state())
+ EXPECT_CALL(*service, state())
.WillOnce(Return(Service::kStateIdle));
- EXPECT_CALL(*service.get(), SetState(_));
- EXPECT_CALL(*service.get(), SetConnection(_));
+ EXPECT_CALL(*service, SetState(_));
+ EXPECT_CALL(*service, SetConnection(_));
EXPECT_TRUE(HasLinkMonitor());
SelectService(NULL);
EXPECT_FALSE(HasLinkMonitor());
@@ -686,6 +691,23 @@
}
TEST_F(DeviceTest, ShouldUseMinimalDHCPConfig) {
+ // With no selected service, we should not choose a minimal config.
+ EXPECT_FALSE(device_->ShouldUseMinimalDHCPConfig());
+
+ MockManager manager(control_interface(),
+ dispatcher(),
+ metrics(),
+ glib());
+ scoped_refptr<MockService> service(
+ new StrictMock<MockService>(control_interface(),
+ dispatcher(),
+ metrics(),
+ &manager));
+ SelectService(service);
+ EXPECT_CALL(*service, ShouldUseMinimalDHCPConfig())
+ .WillOnce(Return(true))
+ .WillOnce(Return(false));
+ EXPECT_TRUE(device_->ShouldUseMinimalDHCPConfig());
EXPECT_FALSE(device_->ShouldUseMinimalDHCPConfig());
}